[UI Framework][K7]: Side nav component #13304
Conversation
| @@ -33,6 +33,9 @@ import LoadingExample | |||
| import PopoverExample | |||
| from '../../views/popover/popover_example'; | |||
|
|
|||
| import SideNavExample | |||
| from '../../views/side_nav/side_nav'; | |||
There was a problem hiding this comment.
This needs to import side_nav/side_nav_example.
There was a problem hiding this comment.
Oh man. I'm a dunce! Thanks.
| }]} | ||
| > | ||
| <GuideText> | ||
| Description needed: how to use the SideNav component. |
There was a problem hiding this comment.
Maybe a better description, or no description at all?
| {/* Hidden from view, except in mobile */} | ||
| <button className="kuiSideNav__mobileToggle kuiLink"> | ||
| <span className="kuiSideNav__mobileTitle">Navigate within management</span> | ||
| <span><KuiIcon className="kuiSideNav__mobileIcon" type="apps" size="medium"/></span> |
There was a problem hiding this comment.
Could you format this and similar JSX like this:
<span>
<KuiIcon
className="kuiSideNav__mobileIcon"
type="apps"
size="medium"
/>
</span>This adheres to our HTML style guide rules. Oddly, the linter should be catching this. Do you have the latest NPM dependencies installed? You can also run npm run lintroller to autofix a lot of this style guide stuff.
| * 1. Animation doesn't work against height. Need max-height instead. | ||
| * We set a value larger than what is needed to fake "auto". | ||
| */ | ||
| .kuiSideNav.isOpenMobile .kuiSideNav__content { |
There was a problem hiding this comment.
Can we rename this to be .kuiSideNav-isOpenMobile?
| <KuiSideNavItem>Users</KuiSideNavItem> | ||
| <KuiSideNavItem>Roles</KuiSideNavItem> | ||
| <KuiSideNavItem isSelected>Watches</KuiSideNavItem> | ||
| <KuiSideNavItem>Extremely long item is long</KuiSideNavItem> |
There was a problem hiding this comment.
Can we make this example much longer, so we can see how content wraps? Maybe it should truncate with an ellipsis instead?
There was a problem hiding this comment.
It does trunctate! That's what this shows. :)
| } | ||
|
|
||
| .kuiSideNav__mobileToggle { | ||
| display: flex; |
There was a problem hiding this comment.
I think we'll need to add a <span> element inside of the <button> and use that as our flexbox container due to problems with buttons being flex elements in some browsers.
There was a problem hiding this comment.
And once we do that, we can add justify-content: space-between to that element, and remove flex-grow: 1 from .kuiSideNav__mobileTitle.
| border: $kuiBorderThin; | ||
| border-color: transparent; | ||
|
|
||
| &.isSelected { |
There was a problem hiding this comment.
Can we make this .kuiSideNavItem-isSelected?
| display: block; | ||
| width: 100%; | ||
| text-align: left; | ||
| @include kuiFontSizeS; |
There was a problem hiding this comment.
Can we put all of our mixins at the top of the class declaration?
| border-left: $kuiBorderThick; | ||
| border-left-color: $kuiColorSecondary; | ||
| background-color: $kuiColorEmptyShade; | ||
| @include kuiBottomShadowSmall; |
There was a problem hiding this comment.
Can we move this to the top?
| margin-top: $kuiSize; | ||
|
|
||
| &:first-of-type { | ||
| margin-top: 0; |
|
Feedback addressed. Merging. |
Adds a responsive side nav component to KUI.
Adds a responsive side nav component to KUI.
Adds a responsive side nav component to KUI.
Adds a side nav component. While building it I realized we needed to fix some spacing / depth issues in the overall design.
Notes