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

added UI changes for API tokens page empty state #308

Merged
merged 4 commits into from
May 16, 2019

Conversation

vinay033
Copy link
Collaborator

Signed-off-by: vinay033 [email protected]

🔩 Description

Currently, when there are no api tokens on the API Tokens page we show an empty table, instead, we would like to follow the empty state pattern.

👟 Demo Script / Repro Steps

Existing UI
reproduce_303

New changes
fixed_303

⛓️ Related Resources

#303

@vinay033 vinay033 self-assigned this May 10, 2019
@susanev susanev self-requested a review May 10, 2019 14:43
@susanev susanev added the ui label May 10, 2019
Copy link
Contributor

@susanev susanev left a comment

Choose a reason for hiding this comment

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

Thank you!!

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.

Thank you! 🎉

</app-authorized>
</chef-toolbar>
<app-authorized [anyOf]="[['/iam/v2beta/tokens', 'get'], ['/auth/tokens', 'get']]">
<chef-table>
<chef-thead>
<chef-tr>
<chef-tr *ngIf="(sortedApiTokens$ | async)?.length > 0">
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 I wonder why we'd put this on chef-tr and not one of the parent tags? app-authorized or chef-table or chef-thead

Copy link
Contributor

@tylercloke tylercloke left a comment

Choose a reason for hiding this comment

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

Tested and added a few suggestions based on @srenatus's comments.

</div>
<div *ngIf="(sortedApiTokens$ | async)?.length > 0">
<chef-button id="create-button" primary (click)="openCreateModal()">Create Token</chef-button>
</div>
</app-authorized>
</chef-toolbar>
<app-authorized [anyOf]="[['/iam/v2beta/tokens', 'get'], ['/auth/tokens', 'get']]">
<chef-table>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<chef-table>
<chef-table *ngIf="(sortedApiTokens$ | async)?.length > 0">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review and suggestions, I will make these changes and update the PR by today itself.

</app-authorized>
</chef-toolbar>
<app-authorized [anyOf]="[['/iam/v2beta/tokens', 'get'], ['/auth/tokens', 'get']]">
<chef-table>
<chef-thead>
<chef-tr>
<chef-tr *ngIf="(sortedApiTokens$ | async)?.length > 0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<chef-tr *ngIf="(sortedApiTokens$ | async)?.length > 0">
<chef-tr>

@vinay033 vinay033 force-pushed the Vinay/MSYS-1021_API_tokens_page_empty_state_changes branch from 73a5f5b to 9e98fee Compare May 13, 2019 12:29
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.

LGTM but I'm no CSS expert

@vinay033
Copy link
Collaborator Author

LGTM but I'm no CSS expert

CSS is only added for showing button in the center if there is no token available.

@vinay033 vinay033 force-pushed the Vinay/MSYS-1021_API_tokens_page_empty_state_changes branch from 9e98fee to e51a736 Compare May 13, 2019 15:54
@msorens
Copy link
Contributor

msorens commented May 13, 2019

Note that for the failing integration test you will need to mock the <app-authorized> component with something like this:

       MockComponent({ selector: 'app-authorized',
                        inputs: ['allOf', 'anyOf'],
                        template: '<ng-content></ng-content>' })
      ],

You can see that in use in components/automate-ui/src/app/page-components/navbar/navbar.component.spec.ts

@vinay033
Copy link
Collaborator Author

Note that for the failing integration test you will need to mock the <app-authorized> component with something like this:

       MockComponent({ selector: 'app-authorized',
                        inputs: ['allOf', 'anyOf'],
                        template: '<ng-content></ng-content>' })
      ],

You can see that in use in components/automate-ui/src/app/page-components/navbar/navbar.component.spec.ts

Thanks for the suggestion, I will check and update the PR.

@vinay033
Copy link
Collaborator Author

vinay033 commented May 13, 2019

MockComponent

@msorens I just checked <app-authorized> component already mocked in components/automate-ui/src/app/pages/api-token/list/api-token-list.component.spec.ts.
in logs show error like

1) Admin pages IAM v1 API Token list control button when clicked, shows Delete Token
--
  | - Failed: element not interactable
  | (Session info: headless chrome=73.0.3683.86)
  | (Driver info: chromedriver=2.46.628388 (4a34a70827ac54148e092aafb70504c4ea7ae926),platform=Linux 4.14.97-90.72.amzn2.x86_64 x86_64)

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.

Ah, yes, you are correct... and then I notice it was a protractor test failing, not a unit test.
Reviewing the test case in question more closely, I did not see any reason for it to fail so I ran it locally, and it passed for me. So you could just do "retry" on the buildkite item, but before you do...

Take a look at my other comment I just posted for a slight optimization. If you make that change, it will of course, redo the buildkite item as well.

@@ -29,11 +29,16 @@
<div class="page-body">
<chef-toolbar>
<app-authorized [anyOf]="[['/iam/v2beta/tokens', 'post'], ['/auth/tokens', 'post']]">
<chef-button id="create-button" primary (click)="openCreateModal()">Create Token</chef-button>
<div *ngIf="(sortedApiTokens$ | async)?.length === 0" class="token-empty-state">
Copy link
Contributor

@msorens msorens May 13, 2019

Choose a reason for hiding this comment

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

Suggested change
<div *ngIf="(sortedApiTokens$ | async)?.length === 0" class="token-empty-state">
<div *ngIf="(apiTokenCount$ | async) === 0" class="token-empty-state">

Same change in all 3 places where the original expression occurred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@msorens I added these changes.

@vinay033 vinay033 force-pushed the Vinay/MSYS-1021_API_tokens_page_empty_state_changes branch from e51a736 to 035d0f2 Compare May 14, 2019 07:18
@alexpop alexpop merged commit d7c222f into master May 16, 2019
@chef-ci chef-ci deleted the Vinay/MSYS-1021_API_tokens_page_empty_state_changes branch May 16, 2019 07:25
@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 ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants