-
Notifications
You must be signed in to change notification settings - Fork 484
Conversation
Can you rebase this? |
c851d5d
to
d9fd41a
Compare
Using the new design, I think we need to add the chevrons for expanding back to the bottom, I'm using it way more often than I realized, especially when dealing with CRDs, and yes, I can learn the keyboard shortcut, but I'm already using my mouse many times and so that ends up being slower. |
@wwitzel3 I still feel that should live in preferences dialog. Combination of preferences dialog plus a shortcut should cover most user cases. Additionally, moving it to the bottom would break Clarity conventions, so I would be reluctant to do that. Bringing it back to the top defies the purpose since it is undeservingly using the prime screen space. Once we have preferences module, maybe this should be one of the frequently used toggles that are placed inside the Preferences Description section? Theme could go there as well... |
How about adding the expand/collapse button as a menu item? Removing the button entirely is likely breaking some clarity convention anyway. |
@GuessWhoSamFoo - what do you exactly mean by menu item? Why would that be better than preferences? |
If we are going to remove the chevrons for menu expansion, then we need the settings / preferences exposed at the bottom of the menu so it can be switched without needing to lookup / known the keyboard short cut, especially for new users. If we want to open that as another issue, that's fine with me. |
The keyboard shortcuts need to indicate that |
That makes sense, I opened #1498 to track UI for handling Preferences. |
web/src/app/modules/sugarloaf/components/smart/navigation/navigation.component.ts
Outdated
Show resolved
Hide resolved
web/src/app/modules/sugarloaf/components/smart/navigation/navigation.component.html
Outdated
Show resolved
Hide resolved
web/src/app/modules/sugarloaf/components/smart/navigation/navigation.component.html
Outdated
Show resolved
Hide resolved
web/src/app/modules/shared/services/navigation/navigation.test.data.ts
Outdated
Show resolved
Hide resolved
web/src/app/modules/sugarloaf/components/smart/navigation/navigation.component.ts
Outdated
Show resolved
Hide resolved
web/src/app/modules/sugarloaf/components/smart/navigation/navigation.component.ts
Outdated
Show resolved
Hide resolved
web/src/app/modules/shared/services/navigation/navigation.service.ts
Outdated
Show resolved
Hide resolved
813e1e3
to
0d18c66
Compare
@GuessWhoSamFoo PTAL |
0d18c66
to
979169e
Compare
web/src/app/modules/shared/services/navigation/navigation.service.ts
Outdated
Show resolved
Hide resolved
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.
lgtm, can you squash before merging?
Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>
Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>
Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>
dcd0781
to
82e441a
Compare
Navigation redesign based on #1353. After going through a few iterations, we settled down on a module based two panel solution that still supports collapsed and expanded state and moves some of the rarely used actions to keyboard shortcuts for now and Preferences in future.
Major changes:
<Ctrl><Shift>N
and will be soon added to Preferences,<Ctrl><Shift>T
,<Ctrl><Shift>L
,Which issue(s) this PR fixes