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

Merged
merged 14 commits into from
Oct 14, 2024
Merged

SAK-50395 Gradebook Migrate from Handsontable to Tabulator #12828

merged 14 commits into from
Oct 14, 2024

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 previously 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 :)

@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
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!

@csev csev added the 25 Blocker label Oct 1, 2024
@adrianfish
Fixed frozen columns.
03f5c56 
Updated movableColumns and resizable options
Drag and drop
Moved from ajax to fetch
@ottenhoff
Copy link
Contributor

Super impressed with the work so far on this. It looks good and this is super encouraging. Big thanks to all testers especially @jesusmmp !

@kunaljaykam
Copy link
Member Author

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!

I have fixed it. Please test it if you get time.

@ottenhoff
Copy link
Contributor

The single click is working great for me. Nice job!

In terms of performance with Chrome on my localhost with MariaDB, 41 columns * 180 students = 7380 gb scores

  • Handsontable: 793ms scripting and 281ms Rendering
  • Tabulator: 609ms scripting and 202ms Rendering

@ern ern merged commit 1a1c548 into sakaiproject:master Oct 14, 2024
5 checks passed
@jesusmmp
Copy link
Contributor

I'll do another round of testing! Thanks @kunaljaykam @ern @ottenhoff

@ottenhoff
Copy link
Contributor

Thanks all for the awesome testing and the many hours of work that everyone put into this. I'm impressed! Big thanks @kunaljaykam

@kunaljaykam kunaljaykam deleted the SAK-50395 branch October 15, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@accesslint accesslint[bot] accesslint[bot] left review comments

@jesusmmp jesusmmp jesusmmp left review comments

@ern ern ern approved these changes

@adrianfish adrianfish adrianfish left review comments

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

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