Jamie/control menu angular ui2 #2283

SEAjamieD · 2019-11-20T00:20:15Z

🔩 Description: What code changed, and why?

An ongoing effort to remove stencil components. The goal of this branch was to remove chef-control-menu from the codebase. It is replaced here with an Angular Material component, specifically mat-select and mat-option. In order to remain keyboard accessible, onClick functions within mat-option have been changed to onSelectionChange per angular material requirement.

⛓️ Related Resources

#2061

👍 Definition of Done

All chef-control-menus have been replaced, slight styling updates to what was requested by UX.

All Control menus are keyboard accessible.

👟 How to Build and Test the Change

build components/automate_ui_devproxy && start_all_services

Across the site, control menus can be found in:
Settings >
notifications
policies
projects
API Tokens
Teams
Users
Node Credentials
Node Integrations

Compliance >
Reports > Nodes Tab
Reports > Profiles Tab
Reports > Controls Tab

✅ Checklist

Tests added/updated?
Docs added/updated?

📷 Screenshots, if applicable

SEAjamieD · 2019-11-20T19:01:59Z

components/automate-ui/src/styles.scss

@@ -196,3 +196,49 @@ chef-loading-spinner[fixed] {
 input {
  line-height: normal;
 }
+


Angular Material requires a lot of over rides, but doesn't really seem to have a good place to target globally outside of the styles.css file. Any suggestions on a better location for something like this. Will get crowded after adding several more components.

SEAjamieD · 2019-11-25T23:11:11Z

@susanev - from a UX/style perspective, this ticket is done. I'm still trying to work through some Cypress testing issues, but I feel comfortable opening up this PR for any style and/or user
related issues you might find.

SEAjamieD · 2019-11-25T23:21:55Z

@susanev - Major apologies! I failed to update to new spec pictured here. On that now and will retag when done

SEAjamieD · 2019-11-26T00:10:11Z

@susanev - Ok, new colors and states are in. Sorry about that!

SEAjamieD · 2019-11-26T17:00:08Z

components/automate-ui/src/styles.scss

+    }
+
+    &:hover {
+      background-color: #DEE5FD;


@susanev several of these colors here are ones that I haven't seen used in the codebase yet. Are there names you want to use for them when I make the variables?

susanev · 2019-11-26T19:13:47Z

@SEAjamieD can we increase the icon size? maybe font-size: 20px; or similar. also can we get the icon centered in the button?

also, we need to be able to tab to these buttons, and open them and select an option with keyboard only. you can tab to them, but there's no visible focus (maybe cause you set the outline to 0?).. and you cant open the menu with ur keyboard.

flow should be..

tab to item
space bar to open
arrow keys to navigate in the menu
enter to select

and if you tab off the menu should close.

SEAjamieD · 2019-12-23T23:28:34Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.html

-                  <chef-option (click)="removeMember(member)">Remove Member</chef-option>
-                </chef-control-menu>
+                <mat-select panelClass="chef-control-menu-x">
+                  <mat-option (onSelectionChange)="removeMember($event, member)">Remove Member</mat-option>


mat-select requires the use of onSelectionChange in order to be keyboard accessible. Unfortunately this will cause a double fire of connected function -> one for the selection and one for the deselection. The current workaround is running an if check for whether on not this was user input using $event.isUserInput. see: angular/components#4094 (comment)

The material docs (https://v6.material.angular.io/components/select/overview)" indicate "mat-select is designed to work inside of a <mat-form-field> element." Not clear what the ramifications of that are. Any potential pitfalls using it without a <mat-form-field> ?

SEAjamieD · 2019-12-23T23:29:19Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.ts

+export interface KeyboardEvent {
+  isUserInput: boolean;
+}
+


By default - isUserInput is not part of the KeyboardEvent interface - this extends it.

Educate me: is isUserInput something you are populating or is it actually provided by the system though not exposed...?

isUserInput is provided by Angular Material. Every time an option is selected there is an event fired for selected and an event fired for deselected. The selected event passed through isUserInput = true and the deselection event passes through isUserInput = false;

SEAjamieD · 2019-12-23T23:30:14Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.ts

+  removeMember($event: KeyboardEvent, member: Member): void {
+    if ($event.isUserInput) {
+      this.store.dispatch(new RemovePolicyMembers(<PolicyMembersMgmtPayload>{
+        id: this.policy.id,
+        members: [member]
+      }));
+    }


An example of using the $event.isUserInput to only get the selection and not the deselection.

SEAjamieD · 2019-12-23T23:31:01Z

components/automate-ui/src/app/modules/token/token-list/api-token-list.component.html

-                  <chef-option (click)="notifyCopy()" data-cy="copy-token">
+              <chef-table-cell class="controls">
+                <mat-select panelClass="chef-control-menu-x" id="menu-{{token.id}}" data-cy="token-control">
+                  <mat-option (onSelectionChange)="notifyCopy($event)" data-cy="copy-token">


Again - instances of passing in the event to check agains isUserInput.

SEAjamieD · 2019-12-23T23:31:21Z

components/automate-ui/src/app/modules/token/token-list/api-token-list.component.ts

+export interface KeyboardEvent {
+  isUserInput: boolean;
+}


Same as above - extending Keyboard Event

See my earlier comment; this should only occur once in a common file.

SEAjamieD · 2019-12-24T00:16:22Z

@susanev

also, we need to be able to tab to these buttons, and open them and select an option with keyboard only. you can tab to them, but there's no visible focus (maybe cause you set the outline to 0?).. and you cant open the menu with ur keyboard.

flow should be..

tab to item
space bar to open
arrow keys to navigate in the menu
enter to select
and if you tab off the menu should close.

How set are you on this flow? With angular material we get most of this built in, only change is that instead of using space bar to open, enter opens. Is this acceptable?

susanev · 2019-12-24T01:05:02Z

@SEAjamieD that's fine, thanks for checking!!

msorens

Initial pass--some items need fixing as noted below.

components/automate-ui/src/app/components/chef-components.module.ts

msorens · 2019-12-24T19:23:11Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.html

-                  <chef-option (click)="removeMember(member)">Remove Member</chef-option>
-                </chef-control-menu>
+                <mat-select panelClass="chef-control-menu-x">
+                  <mat-option (onSelectionChange)="removeMember($event, member)">Remove Member</mat-option>


The material docs (https://v6.material.angular.io/components/select/overview)" indicate "mat-select is designed to work inside of a <mat-form-field> element." Not clear what the ramifications of that are. Any potential pitfalls using it without a <mat-form-field> ?

msorens · 2019-12-24T19:36:58Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.ts

@@ -20,6 +20,10 @@ import {

 export type PolicyTabName = 'definition' | 'members';

+export interface KeyboardEvent {


🤔 You are actually replacing the definition not extending it, and that makes it rather confusing for someone who expects a KeyboardEvent to be the original.
Would suggest rename this and really extend it:

Suggested change

export interface KeyboardEvent {

export interface ChefKeyboardEvent extends KeyboardEvent {

Also, I notice you export KeyboardEvent in two different files; they happen to be the same so no harm done at the moment, but that is brittle.
I would urge moving/consolidating it to a new file under src/app/types and just have a single definition that is imported in the three files that need it.

Finally, would like to add an in-code comment so folks know exactly what it is for: "isUserInput allows filtering to selection events and ignores deselection events".

components/automate-ui/src/app/modules/policy/list/policy-list.component.html

components/automate-ui/src/app/modules/team/team-management/team-management.component.html

msorens · 2019-12-24T19:41:21Z

components/automate-ui/src/app/modules/token/token-list/api-token-list.component.html

-              <chef-table-cell class="three-dot-column">
-                <chef-control-menu id="menu-{{token.id}}" data-cy="token-control">
-                  <chef-option (click)="notifyCopy()" data-cy="copy-token">
+              <chef-table-cell class="controls">


Here is another pattern to look for--this line also needs to be reverted, and again, check for other occurrences. Be sure to check the corresponding scss entry for this, too.

msorens · 2019-12-24T19:49:39Z

components/automate-ui/src/app/modules/token/token-list/api-token-list.component.ts

+export interface KeyboardEvent {
+  isUserInput: boolean;
+}


See my earlier comment; this should only occur once in a common file.

msorens · 2019-12-24T19:50:55Z

components/automate-ui/src/app/modules/policy/details/policy-details.component.ts

+export interface KeyboardEvent {
+  isUserInput: boolean;
+}
+


Educate me: is isUserInput something you are populating or is it actually provided by the system though not exposed...?

msorens · 2019-12-24T19:59:31Z

components/automate-ui/src/app/pages/project/list/project-list.component.html

-                        <chef-option (click)="startProjectDelete(project)" data-cy="delete-project">Delete Project</chef-option>
-                      </chef-control-menu>
+                      <mat-select panelClass="chef-control-menu-x">
+                        <mat-option (onSelectionChange)="startProjectDelete(project)" data-cy="delete-project">Delete Project</mat-option>


If I am following your protocols, need to add guard if ($event.isUserInput)... inside of the target method (startProjectDelete).
And since a global search for .isUserInput only shows that guard presently in 3 files, I believe there are several others to instrument as well.

Per our conversation, please add on this method and check that there are no other missing ones. Thanks!

susanev · 2019-12-24T22:54:37Z

@SEAjamieD it looks and works well.. a few more things

for some reason, the height of a row with a control menu increases from 55px to 76px, since we will have some tables with rows with and without control menus we really want those to be the same height, any way to keep the row with a control menu at the same height as rows without?
the menu style when opened, and with focus on the menu items looks a bit different than what these were originally designed to look like..
- font appears to be roboto but should be muli like the rest of automate
- on open, the first item has the gray background and im not sure why... would prefer no background color until something gets focus or hover
- on hover/focus can we change the background color to our chef-primary-dark and change the font color to white? (i believe these were the styles before converting to angular material)

Signed-off-by: seajamied <[email protected]>

…st row, which is why the test is failing Signed-off-by: seajamied <[email protected]>

Signed-off-by: seajamied <[email protected]>

susanev

one last super small thing, lets get the background color on selection to use the same primary blue as our buttons; thanks for all of your work on this!!

Signed-off-by: seajamied <[email protected]>

msorens

The gating function we talked about (included in my comments below) is the only one of significance, I would say. The rest of my baker's dozen below are nitty-gritty cleanup. On the whole, the code looks great and it works well, so giving the thumbs up!

components/automate-ui/e2e/admin.e2e-spec.ts

components/automate-ui/src/app/modules/policy/details/policy-details.component.ts

components/automate-ui/src/app/modules/policy/list/policy-list.component.html

components/automate-ui/src/app/pages/integrations/list/integrations-list.component.ts

components/automate-ui/src/app/pages/project/list/project-list.component.spec.ts

components/automate-ui/src/styles.scss

msorens · 2020-01-16T23:12:16Z

components/automate-ui/src/styles.scss

+    }
+  }
+
+  &[aria-owns] {  // hacky alternative to active


The comment gives me only a vague idea of justification; might you have a URL reference you could add here?

I've updated the comment so it makes a little more sense. Ultimately, I noticed "aria-owns" being applied and hooked in on it.

aria-owns by definition tells assistive technology that another element being placed on the screen should be treated as a child element of the current element we're interacting with (https://developers.google.com/web/fundamentals/accessibility/semantics-aria/aria-labels-and-relationships#aria-owns)

I'm ultimately utilizing it out of scope because when aria-owns is added, we know that the dropdown panel is open.

msorens · 2020-01-16T23:19:42Z

components/chef-ui-library/src/global/variables.css

@@ -27,6 +27,7 @@
  --blue-de-france: #3DA5FF;
  --gray: #B7BCBC;
  --blue: #174AF0;
+  --palatinate-blue: #0E3DD5;


Wondering what your source is for this color?
Every reference I can find (e.g. https://encycolorpedia.com/search?q=palatinate+blue) shows palatinate blue as #273be2

I got if from the Automate Extended Color Palette.

msorens · 2020-01-16T23:27:41Z

components/automate-ui/src/app/pages/project/list/project-list.component.html

-                        <chef-option (click)="startProjectDelete(project)" data-cy="delete-project">Delete Project</chef-option>
-                      </chef-control-menu>
+                      <mat-select panelClass="chef-control-menu-x">
+                        <mat-option (onSelectionChange)="startProjectDelete(project)" data-cy="delete-project">Delete Project</mat-option>


Per our conversation, please add on this method and check that there are no other missing ones. Thanks!

components/automate-ui/src/app/pages/integrations/list/integrations-list.component.html

Signed-off-by: seajamied <[email protected]>

susanev · 2020-01-17T18:40:28Z

@SEAjamieD you're going to remove that .toml right?

Signed-off-by: seajamied <[email protected]>

SEAjamieD · 2020-01-17T20:30:02Z

@susanev

@SEAjamieD you're going to remove that .toml right?

sorry thank you!

bcmdarroch

Looks good to me! Had a few questions on the code.

I did notice a little weirdness on the focus but doesn't seem like a blocker:

authz.toml

bcmdarroch · 2020-01-17T20:26:17Z

components/automate-ui/src/app/modules/policy/list/policy-list.component.ts

-    this.deleteModalVisible = true;
-    this.policyToDelete = policy;
+  public startPolicyDelete($event: ChefKeyboardEvent, policy: Policy): void {
+    if ($event.isUserInput) {


what does this conditional do? Is it necessary for any control menu item handler?

This is an effect of using Material UI's mat-select component. When a selection is made, it fires off two events instead of just one. One of the events is the item being selected, and the other event is something being DEselected.

Unfortunately, without this gate being added, we'd fire off two events. The workaround is to check for the isUserInput property being true, if it is - that's the selected event we want.

susanev · 2020-01-17T22:50:35Z

thanks for your attention to detail @bcmdarroch!!
we are aware of the focus weirdness and we decided to move forward with it, but hoping that when the ui team has a chance to think more about how we are using material then we can revisit it then.

SEAjamieD commented Nov 20, 2019

View reviewed changes

SEAjamieD marked this pull request as ready for review November 25, 2019 23:08

SEAjamieD self-assigned this Nov 25, 2019

SEAjamieD requested a review from susanev November 25, 2019 23:09

SEAjamieD added automate-ui DO-NOT-MERGE ui WIP labels Nov 25, 2019

SEAjamieD commented Nov 26, 2019

View reviewed changes

susanev removed their request for review December 6, 2019 18:00

SEAjamieD force-pushed the jamie/control-menu-angular-ui2 branch 2 times, most recently from 8d05df1 to f95753b Compare December 20, 2019 21:58

SEAjamieD commented Dec 23, 2019

View reviewed changes

SEAjamieD requested review from susanev, msorens and a team December 23, 2019 23:53

SEAjamieD removed DO-NOT-MERGE WIP labels Dec 24, 2019

msorens reviewed Dec 24, 2019

View reviewed changes

seajamied added 8 commits January 15, 2020 13:51

Fix css lint

d6f52f8

Signed-off-by: seajamied <[email protected]>

WIP - control menu (mat-select) is acutally missing in credentials li…

46fb8fa

…st row, which is why the test is failing Signed-off-by: seajamied <[email protected]>

Make all text in panel Muli

bf9126b

Signed-off-by: seajamied <[email protected]>

Specific centering of control menu across sizes

4fffb17

Signed-off-by: seajamied <[email protected]>

Update hover and focus styles

0011dcd

Signed-off-by: seajamied <[email protected]>

Lint fixes

ab3cbcf

Signed-off-by: seajamied <[email protected]>

Add three dot column class to node credentials

7b578db

Signed-off-by: seajamied <[email protected]>

Remove some commenting

dd1d114

Signed-off-by: seajamied <[email protected]>

SEAjamieD force-pushed the jamie/control-menu-angular-ui2 branch from 3b60a2b to dd1d114 Compare January 15, 2020 22:10

susanev approved these changes Jan 16, 2020

View reviewed changes

seajamied added 2 commits January 16, 2020 13:37

Update background to chef-primary-bright

5b62f8e

Signed-off-by: seajamied <[email protected]>

Update hover to lighten

4b1c140

Signed-off-by: seajamied <[email protected]>

msorens approved these changes Jan 16, 2020

View reviewed changes

msorens mentioned this pull request Jan 17, 2020

Vinay/automate infra proxy org ui #2605

Closed

seajamied added 7 commits January 16, 2020 21:15

Add gate to project-list component and corresponding test

22748e5

Signed-off-by: seajamied <[email protected]>

Deleted commented out code in admin e2e.spec

cc28a43

Signed-off-by: seajamied <[email protected]>

Replace chef-control-menu-x with chef-control-menu across all files

b1fe95f

Signed-off-by: seajamied <[email protected]>

Add type declaration in integrations-list

a287ddc

Signed-off-by: seajamied <[email protected]>

Delete unused mock component

9725e6e

Signed-off-by: seajamied <[email protected]>

Remove repeated three-dot-class

ff7543b

Signed-off-by: seajamied <[email protected]>

Update sass comment on aria-owns

bd77453

Signed-off-by: seajamied <[email protected]>

remove authz.toml

96f4cc0

Signed-off-by: seajamied <[email protected]>

bcmdarroch approved these changes Jan 17, 2020

View reviewed changes

SEAjamieD merged commit 8e488da into master Jan 17, 2020

chef-expeditor bot deleted the jamie/control-menu-angular-ui2 branch January 17, 2020 23:27

SEAjamieD mentioned this pull request Jan 22, 2020

Add three-dot-column class to any columns using control menu #2685

Merged

4 tasks

SEAjamieD mentioned this pull request Jan 29, 2020

Sachin/automate infra proxy server ui #2581

Merged

		@@ -20,6 +20,10 @@ import {

		export type PolicyTabName = 'definition' \| 'members';

		export interface KeyboardEvent {

	export interface KeyboardEvent {
	export interface ChefKeyboardEvent extends KeyboardEvent {

Jamie/control menu angular ui2 #2283

Jamie/control menu angular ui2 #2283

Conversation

SEAjamieD commented Nov 20, 2019 • edited Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

Choose a reason for hiding this comment

SEAjamieD commented Nov 25, 2019

SEAjamieD commented Nov 25, 2019

SEAjamieD commented Nov 26, 2019

Choose a reason for hiding this comment

susanev commented Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD commented Dec 24, 2019

susanev commented Dec 24, 2019

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susanev commented Dec 24, 2019

susanev left a comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susanev commented Jan 17, 2020

SEAjamieD commented Jan 17, 2020

bcmdarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

susanev commented Jan 17, 2020

SEAjamieD commented Nov 20, 2019 •

edited

Loading

susanev commented Nov 26, 2019 •

edited

Loading

SEAjamieD Jan 17, 2020 •

edited

Loading