[ILM] Adjust alignment of field errors + timeline icon #104084
Conversation
sabarasaba
added
Feature:ILM
Team:Kibana Management
|
@elasticmachine merge upstream |
|
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
There was a problem hiding this comment.
Thanks a lot for fixing this, @sabarasaba!
Checked locally and everything looks great, code changes LGTM too 👍
|
@mdefazio Interesting I didn't noticed it when I did the small changes, but you are 100% right they are off by 2px! I tried to replicate it in a codepen to see if it was a EUI bug but seems to be display "correctly" there, but if I try to put the exact same code snippet in kibana the This can be seen in other apps inside kibana, not just in ILM. I'm unsure if this is a kibana or eui problem though, maybe @chandlerprall could shed some light on this?
|
|
I don't see the 2 pixel offset on |
There was a problem hiding this comment.
Screenshot below shows first formRow with flexEnd and the second with flexStart
The change to flexStart is what's causing the 2px offset. And I can see why we made the change to accommodate for the invalid fields.
Instead of modifying the CSS on the EUI side, I think we should consider instead using an append with an EuiPopover. You can see an example of it here.
Unfortunately, the use of two form fields was an error in the original mockup that was carried over into dev (totally my fault—didn't see this error coming).
Below is a screenshot of the append usage on my local branch.
And here is an updated form mockup that shows all fields with their added 'Append' options (first one showing how the help text will now extend beyond the number input)
After writing this, its possible that the button might stand out too much. If this is the case, we could simply apply color="text to the appended button. I think it's a matter of knowing its clickable vs standing out too much.
Example:
|
Correction on the above, it should be |
|
@elasticmachine merge upstream |
|
Thanks for the guidance @mdefazio! I was able to make your suggested implementation using the append prop and EuiPopover 💥
|
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
Thanks for making these changes. One slight nit would be to change the error callout size to small. But not a blocker as it wasn't really the goal of this PR.
Additionally, I think this is probably worth design taking another look at and considering another layout where the inputs aren't so long. (Would someone be entering in a number any larger than 4 numbers typically?). But again, outside of this PR.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @sabarasaba |
There was a problem hiding this comment.
Looks great, @sabarasaba!
I really like the new design and didn't see any regressions while testing locally.
* adjust alignment of field errors + timeline icon * Use options popover instead of two continuous inputs * Set max row size * Reduce error callout size to s Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…7257) * adjust alignment of field errors + timeline icon * Use options popover instead of two continuous inputs * Set max row size * Reduce error callout size to s Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* adjust alignment of field errors + timeline icon * Use options popover instead of two continuous inputs * Set max row size * Reduce error callout size to s Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Fixes #101378
This PR fixes the alignment of field errors in hot-phase rollover when not using default values and the alignment of the trashcan icon against the timeline.
Screenshots