[#12329] Refactoring of sortable tables - Student list #12539
Conversation
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
1 similar comment
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
|
Hey @jasonqiu212 @weiquu @cedricongjh, this PR is ready for review! |
|
Hi @domoberzin, Sorry for the delay! I'll try to review this PR within the next few days |
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
| @@ -116,7 +116,7 @@ public void testAll() { | |||
| ______TS("delete student"); | |||
| detailsPage.sortByName(); | |||
| detailsPage.sortByStatus(); | |||
| StudentAttributes[] studentsAfterDelete = { students[3], students[0], students[1] }; | |||
| StudentAttributes[] studentsAfterDelete = { students[0], students[3], students[1] }; | |||
There was a problem hiding this comment.
could I check why the E2E test has changed?
There was a problem hiding this comment.
I believe this is due to the fact that in the original table, when clicking on sort, it would sort descending by default, while the current implementation would sort it in ascending by default, hence the change in the ordering
cedricongjh
added
s.FinalReview
There was a problem hiding this comment.
LGTM! Thanks for the great work @domoberzin
Seems strange to me that there are 2 differing stylings for the table header - The gray and cyan blue. Perhaps we can consider standardizing it to cyan blue only? What do you think @cedricongjh
jasonqiu212
added
s.ToMerge
|
Hi @domoberzin, Sorry could you help to fix the snapshot tests? I think they're failing after merging in your other refactoring for extension confirm model. |
|
Hi @jasonqiu212, snapshots have been updated |
wkurniawan07
added
the
c.Task
Fixes part of #12329
Outline of Solution
Refactored student list to fully use sortable tables, coupled with minor changes to sortable table to accommodate different styling on the header of the table.
Following the lead of #12501, I created the
cell-with-actionscomponent to support the various buttons on the original table.Also modified the
sortable-tablecomponent to be able to take in acustomHeaderStylethat supports styling of the header, since for the student list, the class used in the header isbg-info(A cyan-ish colour). This allows the class of the header to be customized beyond the standard blue ofbg-primaryor the blank white.As the student list originally supported highlighting, I imported the highlighter pipe directly into
student-list.componentin and set thedisplayValueof theSortableTableCellDatato the highlighted version based on thesearchString.I also believe this refactoring fixes #12536 as shown in the video below.
Screen.Recording.2023-08-01.at.4.01.44.AM.mov
One thing to note is, the original student list had a setting to enable a gray header, which was set in the
<thead>element. While this is probably achievable with some changes within thesortable-tablecomponent, I opted for a slightly simpler solution, to usebg-lightinstead, which is a light gray colour, please refer to the images below and let me know if the colour change is acceptable, otherwise I will attempt to make it closer to the original.Original:
Current:
