-
Notifications
You must be signed in to change notification settings - Fork 208
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
(feat) Even more left nav UI tweaks #647
Conversation
Size Change: -64 B (0%) Total Size: 2.19 MB ℹ️ View Unchanged
|
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.
Looks good to me!
I believe the height should be taken care by size
prop.
Thanks for working on it.
/* Tablet */ | ||
:global(.omrs-breakpoint-lt-desktop) { | ||
:global(.cds--side-nav__link) { | ||
height: 3rem; |
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 believe this should be taken by the size
prop of the menu item instead of CSS, creates confusion when making changes in the component. Faced it when working with tables too.
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 don't see a size
prop https://react.carbondesignsystem.com/?path=/docs/components-ui-shell--fixed-side-nav.
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.
Although, there is a large
prop on side nav links which applies the .#{$prefix}--side-nav__item--large
class which defaults to 3rem
.
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.
Don't see it in the docs. Mind dropping a link, Ian?
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.
Never mind, just seen it
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 SideNavLink
component in the docs you shared. The class I was referring to is defined here
Requirements
For changes to apps
If applicable
Summary
Annoying that we're here again, but I cannot for the life of me figure out why local changes to the styleguide don't get propagated to my dev server after rebuilding the styleguide when running
yarn run:shell
. Regardless, here we are. The basic premise here is that these changes:#f4f4f4
) on focus and hover.#161616
, whereas inactive items have a text color of#525252
.3rem
tall on a tablet, and2rem
tall on a desktop.Other
This is the desired outcome.