-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix CSS style in the admin menu #16923
Conversation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked about turning off these CodeRabbit comments BTW: https://discord.com/channels/1134356397673414807/1134374140397236334/1299013077894828042
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these fix, exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix the bottom padding to the menu from 40px to var(--oc-start-navigation-width-when-compact)
remove the duplicate and invalid https://github.com/OrchardCMS/OrchardCore/pull/16923/files#diff-88900b4eabfa75e6de99b31ed2a1ae5491344b5cfd1a525aa1a1cc07489a292cL1202
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the code diff :). What I'm asking about is, what is the problem that these changes fix? --oc-start-navigation-width-when-compact
is 48px, so where the variable is used now instead, the styling changed. Similarly, the font size will be different, and the padding too, as will the text-shadow
.
The duplicated color
I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the correct value to use is what is stored in the variable and not hardcode 40px; It's better to use rem unit instead of px. bootstrap migrated to use rem which depends on the font I believe. so it's better to use rem not px. the text-shadow I removed it because its the only place we use shadow on a button. not shadow to keep it consistent with what we do everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the value in the variable correct? The variable stores, well, the width of the navbar in its collapsed form. I fail to see how this would be the same as the bottom margin of the menu or the height of the area of the arrow at the bottom. These are different metrics. Please elaborate.
While I'm not an expert on this, yes, using relative font sizes seems to be better, but this should be done in a coherent system. Setting one text to rem
that looks similar in size than the previous pixel value while almost everything in the styling is otherwise expressed in pixels will only make things convoluted.
The text shadow (new-old on the screenshot) I don't know, the old one looks good to me too (the hover style of this arrow is different than the rest of the menu anyway).
Next time, please elaborate these in the PR description if there's no corresponding issue, so we can start from there, instead of having to figure it out during such a conversation.
I really don't see the point of these changes apart from the duplicated color
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll close this PR. We'll fix the color in a different PR and some other time.
I am not sure why running gulp rebuild generate all these files. PR to test