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.

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

synchronize Course/Session objective description/parentObjectives FadeText status #8215

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

Conversation

michaelchadwick
Copy link
Contributor

Fixes ilios/ilios#5645

Whew, this was a long time coming for me. Basically, be it Courses or Sessions, their objectives (and their Description and Parent Objective columns) now synchronize in individual rows, so that if you expand one column, the other column expands, as well (and in the opposite direction, too). Had to learn how to send functions from parent objectives down to child objectives and all that, so fun! ObjectiveSortManager has to short-circuit this by having its own function that overrides the one sent by ObjectiveListItem.

@michaelchadwick michaelchadwick marked this pull request as ready for review November 7, 2024 00:38
@jrjohnson
Copy link
Member

Behavior looks great to me! There are some possibly artistic choices in what expands all. Clicking the expander catches everything, but clicking to edit a title or parents doesn't. I played with it for a while and I think I like this, but wanted to call it out for further testing by @dartajax.

@jrjohnson jrjohnson requested review from dartajax and removed request for stopfstedt November 7, 2024 05:56
@michaelchadwick michaelchadwick force-pushed the frontend-5645-sync-obj-families-expansion-collapsion branch from d425132 to f713d10 Compare November 7, 2024 22:57
@michaelchadwick
Copy link
Contributor Author

@dartajax @jrjohnson I fixed the buggy icon we saw in the standup. I also fixed some icon inconsistencies I saw (per the light on dark vs dark on light styling). I also also fixed how the ObjectiveSortManager displayed so the draggable icon now sits in the center like before. I know that was for a future PR but I figured it out, so :D

@michaelchadwick
Copy link
Contributor Author

Still gotta fix the Session version of these things, tho...

stopfstedt
stopfstedt previously approved these changes Nov 8, 2024
Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

reviewed and click-tested. LGTM.

@stopfstedt stopfstedt dismissed their stale review November 8, 2024 23:23

i spoke too soon. the course objectives version of this works as intended, but session objectives is bugged.

@michaelchadwick michaelchadwick force-pushed the frontend-5645-sync-obj-families-expansion-collapsion branch 4 times, most recently from 96b7d70 to 3765dca Compare November 26, 2024 23:36
@michaelchadwick
Copy link
Contributor Author

Got all the syncing stuff working, and fixed existing tests. Even without Code Climate warning me, I know new tests need to be written to account for the changes, but just want to make sure this is cool so far.

@michaelchadwick michaelchadwick force-pushed the frontend-5645-sync-obj-families-expansion-collapsion branch from 3765dca to 74060e4 Compare December 2, 2024 16:17
@dartajax
Copy link
Member

dartajax commented Dec 2, 2024

image

image

@michaelchadwick
Copy link
Contributor Author

@dartajax that's a valid visual point. I'm not sure yet how to improve the whitespace, but I will look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand/collapse both course/session objective description AND parent objective
4 participants