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

[A2-763] Hide "Access Management" on sidebar #385

Merged
merged 3 commits into from
May 23, 2019

Conversation

blakestier
Copy link

@blakestier blakestier commented May 17, 2019

🔩 Description

Previously, if logged in as an "Editor" on 2.0, we were seeing the "Access Management" title in the sidebar without any subcategories.

With this change, an Editor does not see the title on 2.0. However, on 2.1, the editor can see both the title and "Projects" underneath.

On 2.0:
Screen Shot 2019-05-17 at 1 58 55 PM

On 2.1:
Screen Shot 2019-05-17 at 1 58 29 PM

@blakestier blakestier force-pushed the bs/a2-763/hide-access-management branch from 70a412b to 182c368 Compare May 17, 2019 20:04
@blakestier blakestier changed the title Only show access management if projects visible & 2.1 [A2-763] Hide "Access Management" on sidebar May 17, 2019
@blakestier blakestier removed the WIP label May 17, 2019
@susanev susanev added automate-ui bug 🐛 Something isn't working ui labels May 17, 2019
@@ -26,8 +26,8 @@
<chef-sidebar-entry route="/settings/tokens" icon="vpn_key">API Tokens</chef-sidebar-entry>
</app-authorized>

<div *ngIf="(iamMajorVersion$ | async) !== 'v1'">
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible">Access Management</div>
<div *ngIf="(iamMajorVersion$ | async) === 'v2'">
Copy link
Author

Choose a reason for hiding this comment

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

I could see the case for keeping this as a !== v1 (if we bump to a 3?) . . . I just found the positive === so much easier to read 🤷‍♂

Copy link
Contributor

@msorens msorens left a comment

Choose a reason for hiding this comment

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

Just right!

@@ -26,8 +26,8 @@
<chef-sidebar-entry route="/settings/tokens" icon="vpn_key">API Tokens</chef-sidebar-entry>
</app-authorized>

<div *ngIf="(iamMajorVersion$ | async) !== 'v1'">
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible">Access Management</div>
<div *ngIf="(iamMajorVersion$ | async) === 'v2'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to keep the not-equal-v1 because if we go to v3, etc. we likely want it still to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL -- just refreshed and saw your comment on the very same thing!

Copy link
Contributor

Choose a reason for hiding this comment

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

So I will add in that not-equal-v1 is also consistent with all the other uses in the front-end.

<div *ngIf="(iamMajorVersion$ | async) !== 'v1'">
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible">Access Management</div>
<div *ngIf="(iamMajorVersion$ | async) === 'v2'">
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible && (iamMinorVersion$ | async) === 'v1'">Access Management</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seemed like this was being slightly redundant putting it here--why not just move the one on the chef-sidebar up to its parent app-authorized?--but as we discovered, doing that, and changing this to projects?.visible failed! Seems like the Angular reference variable (projects) is not quite like a real variable after all, so this is a good solution!

@@ -27,7 +27,7 @@
</app-authorized>

<div *ngIf="(iamMajorVersion$ | async) !== 'v1'">
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible">Access Management</div>
<div class="group" *ngIf="policies.visible || roles.visible || projects.visible && (iamMinorVersion$ | async) === 'v1'">Access Management</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, got distracted and forgot one comment.
I would urge adding parentheses around the new expression (projects.visible && (iamMinorVersion$ | async) === 'v1'"). Should not rely on assumptions about operator precedence.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

This is fine, but I wonder a bit if this isn't asking for a general solution -- feels like a general problem: "if any of the sub-categories exist, display the category header"

@srenatus
Copy link
Contributor

ℹ️ I'll rebase out of politeness and merge this when green.

@srenatus srenatus force-pushed the bs/a2-763/hide-access-management branch from 0d66b62 to c3010d5 Compare May 23, 2019 07:18
@srenatus srenatus merged commit 04b4c6b into master May 23, 2019
@chef-ci chef-ci deleted the bs/a2-763/hide-access-management branch May 23, 2019 09:21
@susanev susanev added the auth-team anything that needs to be on the auth team board label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board automate-auth automate-ui bug 🐛 Something isn't working ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants