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

427: fix section selection styling by refactoring ListItem Button #439

Closed jaydonkrooss closed 5 months ago

jaydonkrooss commented 5 months ago

Fixes #427

Fix section selection styling by refactoring ListItem button. In trying to fix styling bugs on Merge Section page, I moved unmerge button to an alternate position. Merge section styling is updated to retain old layout

pushyamig commented 5 months ago

@jaydonkrooss what was the need to move the unmerge button toward right? CCM views are designed based on the UX/UI principles. Personally I prefer to stay like, but I want to understand your thinking here

jaydonkrooss commented 5 months ago

@pushyamig The old unmerge button was automatically placed below the course name because it was a part of the secondaryAction prop of ListItem. ListItemButton has no SecondaryAction prop, so I tried putting unmerge button simply as a child of ListItemButton, but the styling was not exactly the same (number of students text was causing problems with the spacing). This is a result of me messing around to make the button fit well with position:absolute but now that I'm looking at it, I think I could maybe try to replicate the old layout, if we're concerned about making style changes

jaydonkrooss commented 5 months ago

@pushyamig ready for local testing & approval. The new version of the unmerge button now imitates the old layout,

pushyamig commented 5 months ago

I will review this today

pushyamig commented 5 months ago

@jaydonkrooss for Merge section feature, some how grey background when the spinner is rolling the out of view(looks like hidden). This apply to both left and right side of merge section some how this change effected it. Not happening from Feature branch latest

jaydonkrooss commented 5 months ago

@jaydonkrooss for Merge section feature, some how grey background when the spinner is rolling the out of view(looks like hidden). This apply to both left and right side of merge section some how this change effected it. Not happening from Feature branch latest

Good catch! Looks like I made a mistake nesting the backdrop inside the list element. Should be fixed in the latest commit

pushyamig commented 5 months ago

In Merge Section after searching and moving the course sections from left to right, there is usually a thick blue line indicating your section selection for crosslinting and that is missing!

Screenshot 2024-04-24 at 2 12 44 PM

pushyamig commented 5 months ago

Good catch! Looks like I made a mistake nesting the backdrop inside the list element. Should be fixed in the latest commit

The issue seems to be fixed now.

jaydonkrooss commented 5 months ago

In Merge Section after searching and moving the course sections from left to right, there is usually a thick blue line indicating your section selection for crosslinting and that is missing!

I see, sorry I totally missed restoring this with the new component. Should be fixed now