Conversation
Part of #1781 Signed-off-by: Andrew Seigner <[email protected]>
rmars
left a comment
rmars
left a comment
There was a problem hiding this comment.
🥇 I like this a lot! Thank you for separating out NavigationResource and NavigationResources!
| <MenuList dense component="div" disablePadding> | ||
| <NavigationResource type="authorities" /> | ||
| <NavigationResource type="deployments" metrics={allMetrics.deployment} /> | ||
| <NavigationResource type="namespaces" metrics={nsMetrics.namespace} /> |
There was a problem hiding this comment.
if I click on the linkerd namespace, then click back and forth between emojivoto and linkerd, the uri changes, but the component doesn't actually re-render. It does for the other resource types though.
| const { api, classes } = this.props; | ||
|
|
||
| return ( | ||
| <MenuList dense component="div" disablePadding> |
There was a problem hiding this comment.
what do you think about making the first subMenu Item "All Pods" (etc), rather than navigating to the pods list page when you click to expand the submenu? I find it a little disconcerting if I'm, for example, in the middle of a top page, and I just want to see what resources are available in the sidebar, but I get navigated off the current page.
There was a problem hiding this comment.
@rmars agree it's disorienting when navigating the resource tree causes the page to change. in the second commit, i tried adding a small button, visible on hover, that provides the "all" link you described, while keeping the rest of the menu item clickable for navigation only. let me know what you think.
There was a problem hiding this comment.
hmmm do you have objections to just adding an "All s" option as the first option in the list? I think it's weird to have another icon just pop up that you don't know what it does (even if it's the list icon that Resources uses). I feel like having an "All " option makes it very clear as to what is contained in the option.
Having options to click an adjacent icon as well as click a list item also feels weird in that it divides the actions you could take. This introduces more complication than the simple "I click on a menu and then a submenu" flow. What would be the motivations for breaking this flow?
There was a problem hiding this comment.
@rmars Added an "All" list item as you suggested. You're right a random button appearing is a bit confusing. I was trying to avoid having a list item at the top of the menu that's different from the individual resources below it, as I haven't seen that pattern in a tree navigation UI before. That said, this makes the code way simpler, and is the most explicit for the user. Thanks for the thorough eyes!
There was a problem hiding this comment.
yeah, I do wonder how most people actually do this! thanks for adding!
There was a problem hiding this comment.
Looking around for examples:
- the sidebar of the material icons site has an interesting pop-up menu: https://material.io/tools/icons/?icon=airline_seat_legroom_extra&style=baseline
- the cncf site (the Projects menu), does just list "All " as just another list item:
https://www.cncf.io/projects/#
There was a problem hiding this comment.
ooh, thanks for finding these! pop-up style is pretty cool. something to consider down the road.
| <Grid item>{metricToFormatter["SUCCESS_RATE"](sr)}</Grid> | ||
| <Grid item>{_.isNil(sr) ? null : | ||
| <div className={classNames("success-rate-dot", classes[getSuccessRateClassification(sr)])} />} | ||
| <SuccessRateDot sr={sr} /> |
dadjeibaah
left a comment
dadjeibaah
left a comment
There was a problem hiding this comment.
The sidebar is looking great @siggy! I had one comment regarding the "replication controller" menu item
| <NavigationResource type="deployments" metrics={allMetrics.deployment} /> | ||
| <NavigationResource type="namespaces" metrics={nsMetrics.namespace} /> | ||
| <NavigationResource type="pods" metrics={allMetrics.pod} /> | ||
| <NavigationResource type="replicationcontrollers" metrics={allMetrics.replicationcontroller} /> |
There was a problem hiding this comment.
It looks like the Replication Controllers submenu doesn't expand when I am trying to view replication controllers in my Kubernetes cluster. Running the booksapps example should highlights this.
Signed-off-by: Andrew Seigner <[email protected]>
Signed-off-by: Andrew Seigner <[email protected]>
rmars
left a comment
rmars
left a comment
There was a problem hiding this comment.
🐑 🐑 🐕 sweeeeet! thanks for adding this!
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Nice improvement! Looks good to me.
4 participants
Part of #1781
Signed-off-by: Andrew Seigner [email protected]
1st commit
2nd commit
3rd commit