-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Update banner and popup alerts #5672
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! it's so nice to see the alert banners as components and i like the way you replaced so many sass values with variables. i had one non-blocking question, but otherwise it looks ready to 🚢 !
@@ -1,3 +1,4 @@ | |||
.login-form { | |||
box-shadow: $box-shadow, $box-shadow-high; | |||
overflow: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this cause scrolling? Just curious what this change was for.
{{/message-in-page}} | ||
<AlertBanner | ||
@type="warning" | ||
@message="{{warning}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't eval the string so we'll end up with "{{warning}}"
in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm this is fine - it's HTML in the strings we can't have unless we do {{{message}}}
in AlertBanner
<AlertBanner | ||
@type="warning" | ||
@class="is-marginless" | ||
@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}}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I take it back because it totally works 🤯 - this must be a new thing with angle bracket components, but that's awesome.
@type="warning" | ||
@title="Attention" | ||
@message="This {{model.identityType}} is disabled. All associated tokens cannot be used, but are not revoked." | ||
yieldWithoutColumn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be:
yieldWithoutColumn | |
@yieldWithoutColumn={{true}} |
<div class="message is-{{flash.type}}"> | ||
<div class="columns is-mobile is-variable is-1"> | ||
<div class="column is-narrow message-icon"> | ||
<ICon @glyph="{{get (message-types flash.type) "glyph"}}" @size="20" @excludeIconClass="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ICon @glyph="{{get (message-types flash.type) "glyph"}}" @size="20" @excludeIconClass="true" /> | |
<ICon @glyph="{{get (message-types flash.type) "glyph"}}" @size="20" @excludeIconClass={{true}} /> |
I think it'll work either way (since it's truthy), but with bools we normally want curlies around them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - these look great 👌. I left a couple of suggestions but they're not major (and I think you can just accept in the UI if you're ready to merge otherwise).
Thanks! I've got those last couple of properties fixed in my upcoming empty states PR |
<h5 class="title is-5 has-text-{{if (eq flash.type 'warning') 'dark-yellow' flash.type}}"> | ||
{{get (message-types flash.type) "text"}} | ||
</h5> | ||
<span data-test-flash-message-body=true> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ha, didn't catch this one - we use that data-test selector for a lot of the acceptance tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, oops. I had been trying to use the component here and it's already in there, but I somehow missed including it again after I switched back. Got it fixed in the empty states branch
Warning alert banner:
Error alert banner:
Success Popup:
Full width banner on Unseal