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

Clean up error state of Ingest Pipelines drag and drop list items. #137244

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 26, 2022

While investigating #124027, I noticed that the error state for the Grok processor patterns list looked wonky:

image

There's a second error icon that's being manually rendered on top of the error icon built into EUI. That second error icon displays the error message in a tooltip when you hover over it.

I fixed this by removing the manual error icon and using the EuiFormRow component to render the message. I think this is both an aesthetic and a usability improvement because you know longer need to "look with your mouse" to find the error message, you can just look with your eyes.

image

I'm backporting this as far back as is allowed because it's relatively non-invasive and it's a bugfix.

@cjcenizal cjcenizal added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Ingest Node Pipelines Ingest node pipelines management backport:all-open Backport to all branches that could still receive a release labels Jul 26, 2022
@cjcenizal cjcenizal requested review from a team as code owners July 26, 2022 23:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@cjcenizal cjcenizal force-pushed the ingest-pipelines/drag-and-drop-list-error-state branch from 13668c3 to dedac7c Compare July 27, 2022 16:08
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🙌 Inline messages are way more helpful!

Is there any way to keep the drag and remove icon buttons inline with the input when an invalid message is showing instead of keeping them centered?

181127523-4de78c3f-3196-42d3-9cb7-3566e81fa387

Comment on lines 162 to 163
<EuiFlexGroup gutterSize="none" alignItems="center">
<EuiFlexItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's only one flex item now, you can probably remove this flex group/item wrapper too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot!

@cjcenizal
Copy link
Contributor Author

@cchaos I'd love to align those items the way you suggested, but I can't see a way to do it given the structure of the components and the use of flexbox to maintain alignment on the x axis. I was planning on just accepting the misalignment in the error state. What are your thoughts?

@cchaos
Copy link
Contributor

cchaos commented Jul 27, 2022

You should be able to just bump up the button icon size to s to match the height of the compressed input. https://codesandbox.io/s/modest-dust-4m0div?file=/demo.js
Screen Shot 2022-07-27 at 16 16 21 PM

Beneficially, this gives a better (larger) hit target for those buttons.

@cjcenizal
Copy link
Contributor Author

Thanks @cchaos! I couldn't use EuiButtonIcon because it messed up the drag-and-drop behavior. So I added some custom styles to achieve the same affect. Can you take another look?

Comment on lines 22 to 28
.pipelineProcessorsEditor__form__dragAndDropList__grabIcon {
height: $euiSizeXL;
width: $euiSizeXL;
display: flex;
align-items: center;
justify-content: center;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can just add padding since you already know it's always going to be a compressed input with a small icon.

Suggested change
.pipelineProcessorsEditor__form__dragAndDropList__grabIcon {
height: $euiSizeXL;
width: $euiSizeXL;
display: flex;
align-items: center;
justify-content: center;
}
.pipelineProcessorsEditor__form__dragAndDropList__grabIcon {
padding-top: $euiSizeS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's how it looks with just the padding:

image

It gets a little cramped on either side. I think I'd prefer to leave it as-is for symmetry with the EuiIButtonIcon on the other side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, my example assumed you were using gutterSize.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ingestPipelines 453.3KB 453.0KB -341.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Sass and screenshots LGTM

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Changes LGTM, @cjcenizal! Thanks a lot for fixing this, also verified the fix locally.

@cjcenizal cjcenizal added backport:all-open Backport to all branches that could still receive a release and removed backport:all-open Backport to all branches that could still receive a release labels Jul 29, 2022
@cjcenizal cjcenizal merged commit 405c904 into elastic:main Jul 29, 2022
@cjcenizal cjcenizal deleted the ingest-pipelines/drag-and-drop-list-error-state branch July 29, 2022 17:00
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 29, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 29, 2022
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
7.17 Backport failed because of merge conflicts
8.3
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 137244

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jul 29, 2022
kibanamachine added a commit that referenced this pull request Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:all-open Backport to all branches that could still receive a release Feature:Ingest Node Pipelines Ingest node pipelines management release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.3.4 v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants