UI: Add Toolbar #6626
UI: Add Toolbar #6626
Conversation
|
|
||
| &:hover, | ||
| &:active { | ||
| border-color: $grey-light; | ||
| } | ||
|
|
||
| &:hover { | ||
| background-color: $ui-gray-050; |
There was a problem hiding this comment.
This hover state has been a part of the design system for a while but hadn't been updated yet in Vault
| @@ -1,5 +1,5 @@ | |||
| <form {{action (perform lookup) on="submit"}}> | |||
| <div class="field has-addons"> | |||
| <div class="field is-flex"> | |||
There was a problem hiding this comment.
Better for how the Toolbar handles small screens
| @@ -22,6 +22,7 @@ module('Acceptance | secrets/pki/list', function(hooks) { | |||
| await mountAndNav(assert); | |||
| assert.equal(currentRouteName(), 'vault.cluster.secrets.backend.list-root', 'redirects from the index'); | |||
| assert.ok(page.createIsPresent, 'create button is present'); | |||
| await click('[data-test-configuration-tab]'); | |||
There was a problem hiding this comment.
Added these because the Configure link is now only shown when you are viewing the Configuration tab
| @@ -1,20 +1,17 @@ | |||
| {{#if (eq selectedAction 'rotate')}} | |||
| {{#if key.canRotate}} | |||
| <div class="field is-grouped is-grouped-split box is-fullwidth is-bottomless"> | |||
There was a problem hiding this comment.
This is awesome! I left some inline comments / suggestions - let me know and I can help implement them!
I also had a few questions about design - I saw we're using the + icon some places and the > other places, is there any rule for that? I guess the one place it felt weird was enable engine had a + and enable auth method had a > - I would expect those to be the same. (This feels kinda like the create vs. add discussion we had a while back).
Did we want a toolbar on top of the Policy code mirror when you're creating a new policy?
And the last one - the toolbar button components are blue instead of black in the entity and group show pages:
|
@meirish Phew! Still have one Ember question, but I think everything else is there: Here's what is still causing failing tests: The attributes being passed to the |
| @@ -37,6 +37,7 @@ export const GLYPHS_WITH_SVG_TAG = [ | |||
| 'neutral-circled-outline', | |||
| 'perf-replication', | |||
| 'person', | |||
| 'plus-plain', | |||
| @@ -1,3 +1,7 @@ | |||
| .page-header + .tabs-container { | |||
| class="input" | ||
| type="text" | ||
| /> | ||
| <Toolbar @class="toolbar-namespace-picker"> |
There was a problem hiding this comment.
You shouldn't need @ on class for angle bracket components (I know we had some because of an old polyfill we were using that needed it - should be fine to use w/o it though).
| <ToolbarActions> | ||
| <ToolbarLink | ||
| @params={{array "vault.cluster.access.method" model.id}} | ||
| @data-test-backend-view-link=true |
There was a problem hiding this comment.
data attributes don't need the @ those and class and others (aria-*) attributes get treated as normal HTML attributes with angle bracket syntax, they just get passed through and bound automatically - that's something you'd have to set up in curly bracket syntax
There was a problem hiding this comment.
Removing the @ from these instances actually does stop it from feeding through, but that may be because of how we're passing them along in the template. I was able to remove them in the ToolbarLink component, but something about the ToolbarSecretLink component still needs them there to be able to pass them along to SecretLink. Oh well, at least we cleaned it up where we could.
There was a problem hiding this comment.
Oh I bet that has to do with this being tag-less - no tag so it probably skips the binding making these just regular component attrs. Sorry for the bad info!
| @@ -51,8 +51,6 @@ const OIDC_AUTH_RESPONSE = { | |||
| }, | |||
| }; | |||
|
|
|||
| const WAIT_TIME = 50; | |||
There was a problem hiding this comment.
Oh ha, that must've got left in somehow - I guess that means ESLint failure isn't failing the tests either... need to look at that.
There was a problem hiding this comment.
Oh yeah, I meant to ask about that. I was just trusting that we weren't using it like the warning said, but I can put it back if we want to figure that out separately
There was a problem hiding this comment.
Nope, it's good that it's not there! Just should make sure ESLint is working as expected in CI.
|
Merging! Toolbars incoming 🔨 📊 |
This PR consolidates many of the actions across the Vault UI within a toolbar.
Simple example
Here you can see we move the page-level actions below the tabs, which also gives us an opportunity to remove the line between the page title and the tabs, which cleans up the page quite a bit.
Before:
After:
Complicated example
This example highlights what a big problem we had. The toolbar component comes with clear rules from our Design System, and helps make sense of the many actions on this page.
Before:
After:
Not in this PR
The
confirm-actioncomponent has been redesigned to work (and look) better in Toolbars, but that redesign is not included in this PR. It looks a little ridiculous for the moment.This PR also does not add these Toolbar components to the Storybook yet. Since there are several new components, they will need a fair amount of documentation about how they fit together. We plan to add them in a follow-up PR.