Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SAK-50395 Gradebook Migrate from Handsontable to Tabulator #12828

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

SAK-50395 Gradebook Migrate from Handsontable to Tabulator #12828

wants to merge 11 commits into from

Conversation

kunaljaykam
Copy link
Member

@kunaljaykam kunaljaykam commented Aug 27, 2024 •
edited
Loading

accesslint[bot]
accesslint bot reviewed Aug 27, 2024
View reviewed changes
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are accessibility issues in these changes.

...ebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/CourseGradeOverridePanel.html Show resolved Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/panels/ZeroUngradedItemsPanel.html Show resolved Hide resolved
@ern
Copy link
Contributor

ern commented Aug 27, 2024

Remove commented out code and remove unnecessary console.log

@kunaljaykam kunaljaykam force-pushed the SAK-50395 branch 7 times, most recently from 59ce13a to 7410d5d Compare August 30, 2024 18:03
@kunaljaykam kunaljaykam marked this pull request as ready for review August 30, 2024 18:21
jesusmmp
jesusmmp reviewed Sep 2, 2024
View reviewed changes
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/model/GbModalWindow.java
} else if (GbModalWindow.this.returnFocusToCourseGrade) {
target.appendJavaScript("setTimeout(function() {GbGradeTable.selectCourseGradeCell();});");
}
// target.appendJavaScript(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented text

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep it until some testing by QA. After some time I will remove it and probably some codes relateded to this.

adrianfish
adrianfish requested changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few comments. I've not reviewed the entire PR yet but I didn't want to just leave a load of comments all in one bunch. One thing to watch out for is inconsisten indentation. We're using 2 spaces in all the webcomponents and I'm pretty sure the gb js mainly uses 2 spaces. This is a big PR :)

gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html Outdated Show resolved Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html Outdated Show resolved Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html Outdated Show resolved Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.html Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
adrianfish
adrianfish requested changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small changes :)

gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js Outdated Show resolved Hide resolved
adrianfish
adrianfish requested changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small changes :)

kunaljaykam and others added 3 commits September 7, 2024 16:30
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
4605e19 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
49b348e 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
d7aaaa6 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
kunaljaykam and others added 3 commits September 7, 2024 16:34
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
af67be7 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
49984fa 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam @adrianfish
Update gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
bf908b5 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@jesusmmp
Copy link
Contributor

It's not clear the line-through...

image

@jesusmmp
Copy link
Contributor

When clicking on some icon

image

image

@jesusmmp
Copy link
Contributor

When clicking on lock

image

@jesusmmp
Copy link
Contributor

When clicking on x

image

@jesusmmp
Copy link
Contributor

This should be float: right;

image

@jesusmmp
Copy link
Contributor

jesusmmp commented Sep 11, 2024 •
edited
Loading

Students counter is wrong (on first load)

image

@jesusmmp
Copy link
Contributor

Wrong styles when clicking gear icon

image

@jesusmmp
Copy link
Contributor

Now, you have to do double click on each cell to insert a grade... it's more tedious

@jesusmmp
Copy link
Contributor

When you enter a comment

image

@jesusmmp
Copy link
Contributor

Icons gear and comments are overlap

image

@jesusmmp
Copy link
Contributor

Hide category column

image

@jesusmmp
Copy link
Contributor

When clicking on "Filter students" search

image

@kunaljaykam
Copy link
Member Author

Thank you, @adrianfish and @jesusmmp, for your review. I was on a break but am back today. I'll be addressing and responding to all the issues you've mentioned in this PR.

@kunaljaykam
Copy link
Member Author

kunaljaykam commented Sep 12, 2024 •
edited
Loading

Now, you have to do double click on each cell to insert a grade... it's more tedious

Thank you for testing @jesusmmp ! The double-click is required due to Tabulator’s limitation when embedding both a button and an editor in the same cell. In Handsontable, it's possible to achieve single-click functionality for both. I've tried other methods to work around this limitation in Tabulator, I spent days on this but they introduced performance issues.

kunaljaykam and others added 3 commits September 12, 2024 08:50
@kunaljaykam @adrianfish
Apply suggestions from code review
2f67086 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam
push suggestions
2d4d5a8
@kunaljaykam @adrianfish
Apply suggestions from code review
42c97ec 
Co-authored-by: Adrian Fish <adrian.r.fish@gmail.com>
@kunaljaykam
Copy link
Member Author

Icons gear and comments are overlap

image

@jesusmmp Did you re-deploy the library as well?

@jesusmmp
Copy link
Contributor

Icons gear and comments are overlap
image

@jesusmmp Did you re-deploy the library as well?

I'll do another test round next week!.

Great work! :)

@jesusmmp
Copy link
Contributor

Now, you have to do double click on each cell to insert a grade... it's more tedious

Thank you for testing @jesusmmp ! The double-click is required due to Tabulator’s limitation when embedding both a button and an editor in the same cell. In Handsontable, it's possible to achieve single-click functionality for both. I've tried other methods to work around this limitation in Tabulator, I spent days on this but they introduced performance issues.

Oh!. This is really a big problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ern ern ern left review comments

@jesusmmp jesusmmp jesusmmp left review comments

@accesslint AccessLint accesslint left review comments

@adrianfish adrianfish adrianfish requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kunaljaykam @ern @jesusmmp @adrianfish