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

UI: HDS adoption replace AlertBanner part 1 #21163

Conversation

hellobontempo
Copy link
Contributor

new component

Screenshot 2023-06-12 at 11 49 51 AM

old component

Screenshot 2023-06-12 at 11 49 39 AM

@hellobontempo hellobontempo added this to the 1.15 milestone Jun 13, 2023
@hellobontempo hellobontempo changed the base branch from main to i/VAULT-16910/hds-adoption-replace-AlertBanner June 13, 2023 04:34
}}
</A.Description>
<A.Description class="has-top-margin-xs">
<DocLink @path="/vault/tutorials/enterprise/hashicorp-enterprise-license">
Copy link
Contributor Author

@hellobontempo hellobontempo Jun 13, 2023

Choose a reason for hiding this comment

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

Not using the HDS standalone link with the goal being to use that component within <DocLink> itself

@@ -1,4 +1,6 @@
<AlertBanner @type="warning" class="is-marginless" @title="Vault is sealed" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't transfer is-marginless because HDS::Alert already is

data-test-whitespace-warning
/>
</div>
<Hds::Alert
Copy link
Contributor Author

@hellobontempo hellobontempo Jun 13, 2023

Choose a reason for hiding this comment

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

Removed div and css because looked fine without:
Screenshot 2023-06-13 at 4 14 18 PM

{{/if}}
<NamespaceReminder @mode="edit" @noun="secret" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This being in the middle looked a little wild:
image

'MMM d, yyyy'
}}. Add a new license to your configuration and restart Vault."
class="message-marginless"
<div class="license-banner-wrapper">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-13 at 4 32 51 PM

@message="You are creating a new version based on data from Version {{@model.selectedVersion.version}}. The current version for {{@model.id}} is Version {{@model.currentVersion}}."
/>
</div>
{{#if (or (eq @canReadSecretData false) this.isCreateNewVersionFromOldVersion)}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

combined banners into one message (pattern we used in client count dashboard)
Screenshot 2023-06-13 at 4 23 44 PM

<A.Title>Warning</A.Title>
<A.Description>
This role has more than one
<code>credential_type</code>, currently:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding this code block!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for noticing! I thought it was a nice touch! ✨

@@ -329,10 +329,10 @@ module('Integration | Component | pki issuer cross sign', function (hooks) {
.dom(`${SELECTORS.signedIssuerRow()} [data-test-icon="alert-circle-fill"]`)
.exists('row has failure icon');
assert
.dom('[data-test-alert-banner="alert"] .message-title')
.dom('[data-test-alert-banner] .message-title')
Copy link
Contributor

@kiannaquach kiannaquach Jun 14, 2023

Choose a reason for hiding this comment

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

I'm wondering if we should have an alert SELECTORS file that contains common alert banner elements like [data-test-alert-banner] .message-title, [data-test-alert-banner] .alert-banner-message-body, [data-test-alert-banner]

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'm glad you brought this up! I noticed in the past that general selectors tended to be harder to leverage in tests because they'd appear a bunch of times. Especially with components like alerts where multiple may appear on a page. I'm hoping to actually remove the general data-test-alert-banner all together and have each component reference a unique selector. Ideally this also makes debugging test easier!

Copy link
Contributor

@kiannaquach kiannaquach left a comment

Choose a reason for hiding this comment

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

Everything lgtm! Thanks for leaving the comments with images made the code review easier to go through! ☺️

@hellobontempo hellobontempo merged commit 247ddea into ui/VAULT-16910/hds-adoption-replace-AlertBanner Jun 14, 2023
@hellobontempo hellobontempo deleted the ui/VAULT-17202/replace-AlertBanner-part-1 branch June 14, 2023 21:39
hellobontempo added a commit that referenced this pull request Jun 21, 2023
* UI: HDS adoption replace AlertBanner part 1 (#21163)

* rename test selector

* replace db banner

* add class

* replace db role edit

* db creds

* generate creds

* simpler class

* license banner component

* oidc callback plash

* raft

* aws

* secret create or update

* change to compact alert for form field

* change back to inline

* combine alert banners

* wrap in conditional

* remove references to message class

* UI: HDS adoption replace AlertBanner part 2 (#21243)

* token expire warning

* delete css

* edit form

* item details distribute mfa step 2 transit verify

* back to secondary

* distribute

* oidc lease error

* sign

* kv obj and repl dash

* more repl

* update test selector

* show, creds

* shamir

* pki csr

* pki banners

* add hds library to ember engines

* woops comma

* fix k8 test

* update message error component for last!

* hold off MessageError changes until next pr

* revert test selectors

* update pki tests

* UI: part 3 remove alert banner (#21334)

* final component swap

* and actual final of MessageError

* update MessageError selectors

* delete alert-banner and remove references

* update next step alerts to highlight color

* finishing touches, auth form test and client dashboard inline link

* fix more selectors

* fix shamir flow test

* ui: part 4 final cleanup (#21365)

* replace AlertPopup

* add test tag

* move tag

* one more message error tag

* delete alert popup

* final css cleanup

* move preformatted flash into <p> tag

* ui: address comments for sidebranch  (#21388)

* add periods, move link to trailing

* more periods and typo fix
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