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

reduce padding on LG members/subgroups when none have been added #8061

Conversation

michaelchadwick
Copy link
Contributor

@michaelchadwick michaelchadwick marked this pull request as ready for review August 12, 2024 14:51
Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

I like it. The change exposes an issue in our styles that I messed up 3 years ago which needs to get fixed. Sorry.

@@ -2,4 +2,8 @@

.list {
@include m.main-list-box-table;

&.empty {
Copy link
Member

Choose a reason for hiding this comment

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

You put this in the right place to work, but this style is coming from the wrong file.

It's outside the scope of this PR but this programs/list.scss style should be scoped down to only target the program list and not every .list in the application (everything should be under a .programs-list{ class). Can you make that change here or circle back and make a PR just to do that and then re-do the style in this one? It will also require creating a component style for the learner group list (and possibly other lists which are also accidentally replying on this style).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's chat about this at our next meeting.

@michaelchadwick michaelchadwick force-pushed the frontend-5633-lg-member_subs-style-remove-padding branch from 667f037 to 9c77536 Compare August 21, 2024 18:52
@michaelchadwick
Copy link
Contributor Author

@jrjohnson Hoo boy. Trying fix the change requested led me down a rabbit hole of components and styles and mixins. I think I was able to put on the brakes and just fix the things requested, but I may have threw in something that was needed, even if a bit out of scope for this task. The real fix would take much longer and will have to wait.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

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

I was worried it would be one of those things, this looks great now. Thanks for taking the extra step.

@jrjohnson jrjohnson added the ready to merge PR can be merged when all requirements are met label Aug 23, 2024
@jrjohnson jrjohnson removed the request for review from stopfstedt August 23, 2024 20:17
@dartajax dartajax merged commit ff25fbc into ilios:master Aug 23, 2024
43 of 45 checks passed
@michaelchadwick michaelchadwick deleted the frontend-5633-lg-member_subs-style-remove-padding branch September 13, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR can be merged when all requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minor Formatting Adjustment - Learner Group Management
3 participants