-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Add button to fail running deployments #9831
Conversation
As suggested in #9826. The placement is currently ridiculous.
Ember Asset Size actionAs of 13ab142 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
@DingoEatingFuzz clarified to me that the deployments page is meant to be a historical overview vs a place to change things.
As sensibly suggested by @picatz
Since isInfoAction was only being used in the one place and I needed a different set of overrides for this context, it seemed to me sensible to decompose the overrides so they can be individually set in the invocation vs being bundled together under a not-totally-clear name.
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 is a welcome addition. I have a couple thoughts, dunno how blocking they are, but I think they're at least worth talking through.
- The
padding-bottom
specific value seems brittle. Ideally we can achieve the baseline matching in a way that still works if we change the button styles or button size. - Changing to the classes hash (which is a neat implementation, btw) really opens up the surface area of the component's API. I'm not saying it's wrong, but it gave me pause.
&.has-text-inline { | ||
top: unset; | ||
bottom: 0; | ||
padding-bottom: 6.462px; |
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.
Hmm, is this to match baselines? I wonder if there's a way to do with either line-height
or vertical-align
that won't require such a specific value.
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.
yeah, that was admittedly ridiculous 😳 I couldn’t get those to work but what do you think about this?
&.has-inline-text {
display: inline-flex;
align-items: center;
button {
margin-left: 0.3rem;
}
.confirmation-text {
position: static;
margin-right: 0.2rem;
}
}
I’ve learned while looking at this that the spaces between the button are true spaces, not CSS spaces, but with flexbox I added the 0.3rem
margins to bring back the space 🧐
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 dig it! There is also the hip new ch
unit that is based on character width that may be worth experimenting with. With a variable width font I don't think this is as simple as 1ch
, you'd probably still need some fraction. And at that point maybe it doesn't even matter... I dunno.
agreed that I was reluctant to add another set of conditionals in each of those classes, but they could be extracted to computed properties instead. It seemed nice to me to be able to use |
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.
Reading through the @classes
approach, I'm more comfortable with it this time. I think your summary of it being styling the individual pieces without having to fuss with structure sold me on it.
My root concern was opening up the API to the point where we aren't really providing a constraint in the way a style guide/design system should. But since the structure and components are fixed and only the individual modifiers are consumer-editable I think this is good 👍
This is a less magic number-y approach than before, but since the buttons were previously spaced with an actual space character rather than CSS, some margins are needed to replicate that in the flexbox layout.
77f249d
to
13ab142
Compare
Thanks for the rereview! 💞 I I encountered this test failure but it’s surely unrelated and it disappeared when I reran, so I’ll look at it separately. |
This is a supplement to #9831 to incorporate the extracted missing-permissions error handling from #9909. It fixes this failure on the main branch! 😳 https://app.circleci.com/pipelines/github/hashicorp/nomad/14728/workflows/4c147dca-fd1e-4de7-86aa-90ded7aabad2/jobs/137137
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
As suggested in #9826. As a GIF from the preview deployment:
This necessitated some customisation options for
TwoStepButton
. One isinlineText
, which puts the confirmation text in the same line as the buttons. Also, there was a single-use configuration option namedisInfoAction
that I removed in favour of passing a set of class configuration options like this:This seemed preferable to me to adding another opaquely-named flag that bundles together a set of class overrides for each part of the button, but I’m open to other approaches!
The new two-step-button stories are here and here in the preview deployment.