Skip to content
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

Namespace Picker Updates #19753

Conversation

zofskeez
Copy link
Contributor

@zofskeez zofskeez commented Mar 24, 2023

The following updates have been made to the namespace picker component for use in the sidebar:

  • always display the namespace when user has access to the feature (defaults to root)
  • updates icons for current selected namespace
  • removes back action from current namespace heading
  • removes green check icon from current namespace in menu
  • updates menu styling

image
image

@zofskeez zofskeez added this to the 1.14 milestone Mar 24, 2023
Comment on lines -14 to -18
@include from($mobile) {
margin-left: -$spacing-xs;
padding: $spacing-xxs 0 $spacing-xxs $spacing-s;
width: auto;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the sidebar collapses on smaller breakpoints we don't need the namespace picker to be responsive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love to hear this!

Comment on lines -1 to -5
{{#if (and (not this.accessibleNamespaces.length) this.inRootNamespace)}}
<div class="namespace-picker no-namespaces">
{{! Just yield the logo if they're in the root namespace and only have access to it }}
{{yield}}
</div>
Copy link
Contributor Author

@zofskeez zofskeez Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The namespace picker is now always displayed. Namespaces the user does not have access to will not show up in the menu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are no namespaces? Does it still show up but with nothing to select? Do we have an empty state message telling the user why they don't see anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the default state to display as root.

@zofskeez zofskeez marked this pull request as ready for review March 30, 2023 18:48
@@ -5,65 +5,33 @@

.namespace-picker {
position: relative;
color: $white;
color: var(--token-color-palette-neutral-300);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think I should make a ticket to replace all vault colors with hds colors... just thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that will be a part of the HDS adoption epic work. I'm not sure if there are individual tasks for that yet or not.

@@ -95,6 +60,11 @@

.namespace-manage-link {
Copy link
Contributor

@Monkeychip Monkeychip Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance this could easily be solved with a helper instead of overriding the button for this specific case? We override elements and main classes a lot in components, and I want to push for helpers when practical.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, I wrote that CSS so blame me 100%)

@@ -112,6 +82,11 @@
.namespace-link.is-current {
margin-top: $size-8;
margin-right: -$size-10;

svg {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above.

@include from($mobile) {
margin-left: $size-10;
}
margin-left: $spacing-xs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being a little pedantic here, but can we use a helper if we have one for the margin situation? We also may have a font-size helper as well. If it's a pain, ignore me, just pushing helper usage instead of component specific styling when I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to get too in the weeds with removing classes in favor of helpers since there is already a namespace stylesheet. I'm not sure if we have fully fleshed out the plan for updating styling post bulma yet. FWIW it doesn't look like there is a 1rem font size helper class.

<div class="is-mobile level-left">
{{#unless this.isUserRootNamespace}}
<NamespaceLink
@targetNamespace={{or
(object-at (dec 2 this.menuLeaves.length) this.lastMenuLeaves)
this.auth.authData.userRootNamespace
}}
@class="namespace-link is-current button is-ghost icon"
@class="namespace-link is-current button icon is-transparent"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have an icon.is-blue and a button.is-transparent class—not very consistent, I know. Maybe it's the ordering of the class names, what do you think about having the icon come last because it's really the button.is-transparent that is going to do some styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I can update that. Just for future thought, if we are interested in class name order on elements we should automate something to enforce that (alphabetical or something else) since everyone will put them in likely a random order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something else to maybe keep in mind while we establish our updated styling patterns is that something like a button-transparent class that inherits the button styling and adds the transparency is probably more descriptive than having a button and is-transparent class if is-transparent only applies to buttons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and yes. Totally agree.

<div
class="leaf-panel
{{if (eq leaf this.currentLeaf) 'leaf-panel-current' 'leaf-panel-left'}}
{{if (and this.isAdding (eq leaf this.changedLeaf)) 'leaf-panel-adding'}}
{{if (and (not this.isAdding) (eq leaf this.changedLeaf)) 'leaf-panel-exiting'}}
"
>
{{~#each-in (get this.namespaceTree leaf) as |leafName|}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did that each even work with the ~? Does this mean something I don't know about or was this an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I was surprised by that too! I'm pretty sure that used to be a private helper (maybe)? Not sure why that wasn't raising a warning.

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking just a couple of scss things. One question, what do you think about moving the glimmerizing the namespace picker into this project? Would having that updated help your development or hinder it? I feel like even just having the documentation would be helpful.

@zofskeez
Copy link
Contributor Author

One question, what do you think about moving the glimmerizing the namespace picker into this project? Would having that updated help your development or hinder it? I feel like even just having the documentation would be helpful.

Great question! I thought about it. Several times actually 😁. I ended up feeling like it was out of scope since these were primarily style/visual changes. I was preparing to take it on if we were going to move forward with the functional updates as part of this project. I created a ticket to update the navigation and added a suggestion to the description to glimmerize at that time. The component logic is 5 years old and was concerned that refactoring could lead to bugs that are unrelated to the sidebar nav work. Some of the code to generate the menu links seemed like it could be simplified which I would want to update as part of the component refresh.

@zofskeez zofskeez merged commit fbd517f into ui/VAULT-12799/sidebar-navigation Mar 30, 2023
@zofskeez zofskeez deleted the ui/VAULT-14459/namespace-picker-updates branch March 30, 2023 21:28
zofskeez added a commit that referenced this pull request May 3, 2023
* Add Helios Design System Components (#19278)

* adds hds dependency

* updates reset import path

* sets minifyCSS advanced option to false

* Remove node-sass (#19376)

* removes node-sass and fixes sass compilation

* fixes active tab li class

* Sidebar Navigation Components (#19446)

* links ember-shared-components addon and imports styles

* adds sidebar frame and nav components

* updates HcNav component name to HcAppFrame and adds sidebar UserMenu component

* adds tests for sidebar components

* fixes tests

* updates user menu styling

* fixes typos in nav cluster component

* changes padding value in sidebar stylesheet to use variable

* Replace and remove old nav components with new ones (#19447)

* links ember-shared-components addon and imports styles

* adds sidebar frame and nav components

* updates activeCluster on auth service and adds activeSession prop for sidebar visibility

* replaces old nav components with new ones in templates

* fixes sidebar visibility issue and updates user menu label class

* removes NavHeader usage

* adds clients index route to redirect to dashboard

* removes unused HcAppFrame footer block and reduces page header top margin

* Nav component cleanup (#19681)

* removes nav-header components

* removes navbar styling

* removes status-menu component and styles

* removes cluster and auth info components

* removes menu-sidebar component and styling

* fixes tests

* Console Panel Updates (#19741)

* updates console panel styling

* adds test for opening and closing the console panel

* updates console panel background color to use hds token

* adds right margin to console panel input

* updates link-status banner styling

* updates hc nav components to new API

* Namespace Picker Updates (#19753)

* updates namespace-picker

* updates namespace picker menu styling

* adds bottom margin to env banner

* updates class order on namespace picker link

* restores manage namespaces refresh icon

* removes manage namespaces nav icon

* removes home link component (#20027)

* Auth and Error View Updates (#19749)

* adds vault logo to auth page

* updates top level error template

* updates loading substate handling and moves policies link from access to cluster nav (#20033)

* moves console panel to bottom of viewport (#20183)

* HDS Sidebar Nav Components (#20197)

* updates nav components to hds

* upgrades project yarn version to 3.5

* fixes issues in app frame component

* updates sidenav actions to use icon button component

* Sidebar navigation acceptance tests (#20270)

* adds sidebar navigation acceptance tests and fixes other test failures

* console panel styling tweaks

* bumps addon version

* remove and ignore yarn install-state file

* fixes auth service and console tests

* moves classes from deleted files after bulma merge

* fixes sass syntax errors blocking build

* cleans up dart sass deprecation warnings

* adds changelog entry

* hides namespace picker when sidebar nav panel is minimized

* style tweaks

* fixes sidebar nav tests

* bumps hds addon to latest version and removes style override

* updates modify-passthrough-response helper

* updates sidebar nav tests

* mfa-setup test fix attempt

* fixes cluster mfa setup test

* remove deprecated yarn ignore-optional flag from makefile

* removes another instance of yarn ignore-optional and updates ui readme

* removes unsupported yarn verbose flag from ci-helper

* hides nav headings when user does not have access to any sub links

* removes unused optional deps and moves lint-staged to dev deps

* updates has-permission helper and permissions service tests

* fixes issue with console panel not filling container width
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants