fix(core/pipeline): make revision dropdown usable, layout tweaks #7569
Conversation
erikmunson
requested review from
alanmquach,
christopherthielen and
caseyhebebrand
|
forgot to add a screenshot of the vertical misalignment, which only happens when you select the current version and the 'restore this version' button is not present: |
| font-size: 120%; | ||
| font-weight: 600; | ||
| letter-spacing: -1px; | ||
|
|
There was a problem hiding this comment.
prettier was extremely unsatisfied with this file.
| @@ -39,48 +46,58 @@ | |||
|
|
|||
| .diff-view { | |||
| display: block; | |||
|
|
|||
| .diff { | ||
| color: var(--color-black); | ||
| font-size: 80%; | ||
| overflow-x: visible; | ||
| margin: 0; | ||
| padding-bottom: 0; | ||
|
|
| .match, | ||
| .add, | ||
| .remove { | ||
| font-family: @font-family-monospace; | ||
|
|
| &::before { | ||
| position: relative; | ||
| left: 0; | ||
| } | ||
| } | ||
|
|
| .form-group { | ||
| margin-bottom: 0; | ||
| } | ||
|
|
| &.add { | ||
| background: var(--color-success); | ||
| } | ||
|
|
| .delta { | ||
| width: 100%; | ||
| position: absolute; | ||
| min-height: 3px; | ||
|
|
| @@ -89,13 +106,16 @@ | |||
| top: 0; | |||
| right: -7px; | |||
| height: calc(~'100% - 8px'); | |||
|
|
|||
| <div className="history-header row"> | ||
| <div className="col-md-4"> | ||
| <div className="history-header row horizontal"> | ||
| <div className="revision-section col-md-4"> | ||
| <FormField | ||
| label="Revision" |
There was a problem hiding this comment.
this might be one of those cases where it would make sense to write an inline layout component instead of fight the css/markup in StandardFieldLayout (css which is likely going to change in the future).
something like this might do the trick:
layout={(label, input) => <div>{label}{input}</div>}
There was a problem hiding this comment.
you know, i originally thought 'aw man i'm gonna need my own layout', and then i had a spark of memory and said 'oh wait Chris overrode the min-width of this label in the Triggers component and it worked out great, i'm gonna steal that'. Guess I should've just tried out the "dumb" layout first.
There was a problem hiding this comment.
Turns out to get this looking right you actually need a fair amount of layout code (side-by-side flex with the right margin, bold text, force the input to grow to size), which makes the markup look slightly closer to writing a standard layout:
layout={({ label, input }) => (
<div className="flex-container-h baseline margin-between-lg">
<div className="bold">{label}</div>
<div className="flex-grow">{input}</div>
</div>
)}I'm cool with/prefer that, but do you personally prefer having this level of layout smarts hard-coded inline versus needing to refactor CSS overrides later?
There was a problem hiding this comment.
The idea behind the field layouts is that there are a lot of things that look similar, and we can change them all in lockstep by updating the layout component. I think overusing the layouts for things which dont look similar will make it harder to iterate on the layout itself down the road. I'd lean towards adding the markup.
In fact, FormikFormField components make it really easy to: reuse a standard layout, apply validation, handle state management. FormField components make it easy to reuse a layout, and apply validation. In this UI, the select widget doesn't really benefit from any of those things. Maybe FormField isn't the right choice here at all here, and it should just be markup with a select inside it.
<div className="flex-container-h baseline margin-between-lg">
<div className="bold">Revision</div>
<div className="flex-grow"><SelectInput ... /></div>
</div>There was a problem hiding this comment.
'oh wait Chris overrode the min-width of this label in the Triggers component and it worked out great, i'm gonna steal that'
It's not clear to me if this is abusing the layout or not. I suspect it's borderline abuse.
There was a problem hiding this comment.
yeah, it's not clear to me that the field components here are providing a ton of value. For now I'm gonna leave them as-is since this was mostly a quick dive in and out to fix this layout situation, but I'm certainly happier with the inline layout than coupling overrides to a field layout. Thanks for catching it 🙏
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (#7569) ca176fc feat(provider/aws): Functions (listing and searching) (#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (#7536)" (#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (#7562) 86a365b feat(provider/aws): Functions (listing and searching) (#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (#7563)
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
d71daa5 fix(core/pipeline): fully re-render list of trigger configs after a delete (spinnaker#7571) 629a98f fix(core/pipeline): make revision dropdown usable, layout tweaks (spinnaker#7569) ca176fc feat(provider/aws): Functions (listing and searching) (spinnaker#7568) e49ffaf Revert "feat(provider/aws): Functions (listing and searching) (spinnaker#7536)" (spinnaker#7567) 114303a fix(kubernetes): disable project cluster filtration by stack/detail (spinnaker#7562) 86a365b feat(provider/aws): Functions (listing and searching) (spinnaker#7536) 6236a9f fix(core/pipeline): make UX less bad when a pipeline stage never happened (spinnaker#7563)
When the pipeline revision history dropdown got moved to react, we started using
StandardFieldLayoutwhich has a pesky min width on its label. That caused some "fun" usability challenges with the width of the dropdown itself, which this fixes. I've also taken the liberty of improving some other layout nits, as the diff summary was both vertical misaligned and had a bunch of excess padding/margin that made it look a little funky.Before:
After:
