search

tl-its-umich-edu / canvas-course-manager-next

Canvas Course Manager Next: A redesign of the existing CCM application. It extends Canvas features, makes cumbersome features easier to use, and adds new features.
8 stars 9 forks source link

Change list structure and otherwise improve `SectionSelectorWidget` (#230, #260) #334

Closed ssciolla closed 2 years ago

ssciolla commented 2 years ago

This PR aims to resolve #230 and #260.

Change(s):

  1. Rather than relying on the provided button prop on ListItem, a ButtonBase was placed inside each ListItem. This follows the preferred approach identified by the accessibility team meant to resolve #260.
  2. Modified the usePromise hook for unmerging sections so an intermediate state variable (sectionsToUnmerge) and useEffect hook could be removed.
  3. Made the text of the course name in a selected section blue, which I believe was the original intention, but wasn't happening before (reason...)
  4. Used conditional rendering in a handful of instances (props.multiSelect, props.search, props.header), instead of using hidden styles (this resulted in some minor visual changes). I think this also had the side effect of fixing #230.
  5. Broke up long lines for readability.
pushyamig commented 2 years ago

@ssciolla This fix seem to be working fine. I know for the structure for the table component for screen readers, you shared this PR with Accessibility team and I guess they seems to like it. I don't have anymore feedback on that.

This change seems knowing and unknowingly had few other fixes and code refactoring. Smoke testing all of them working fine. I tested the section widget component with Merge section and add UM user. The items on there header, selectAll checkbox and search are working. I think exhaustive testing might be not needed, just went by code logic.

ssciolla commented 2 years ago

Thanks for the review, @pushyamig. I was looking over the code one more time and doing more testing, and I found a couple other things worth changing, as well as a margin inconsistency (extra margin around the button inside li) with the component in Merge Sections because of a CSS class set in the MergeSections.tsx page. I moved things around a little and resolved it. If you could take a look at my latest commits, I'd appreciate it!