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

[Unified Search] Change filter styling #200675

Conversation

kowalczyk-krzysztof
Copy link
Member

@kowalczyk-krzysztof kowalczyk-krzysztof commented Nov 19, 2024

Summary

This PR changes filter styling to match suggestion proposed here.

Closes: #158875

Visuals

Previous New
before new

@kowalczyk-krzysztof kowalczyk-krzysztof added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Feature:Unified search Unified search related tasks labels Nov 19, 2024
@kowalczyk-krzysztof kowalczyk-krzysztof self-assigned this Nov 19, 2024
@kowalczyk-krzysztof kowalczyk-krzysztof requested a review from a team as a code owner November 19, 2024 07:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@kowalczyk-krzysztof kowalczyk-krzysztof requested review from a team as code owners November 19, 2024 09:59
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@ghudgins
Copy link
Contributor

@kowalczyk-krzysztof I played with it and I don't think it's a good UX to use the tag. can we bold the AND's and ORs instead? I think that could at least nudge it in the right direction

image

thanks for flagging it

@kowalczyk-krzysztof
Copy link
Member Author

kowalczyk-krzysztof commented Nov 21, 2024

@kowalczyk-krzysztof I played with it and I don't think it's a good UX to use the tag. can we bold the AND's and ORs instead? I think that could at least nudge it in the right direction
image

thanks for flagging it

@ghudgins Sure, I got rid of the badge and made it so it's EuiTextColor again with same color it was but bold.

new

@kowalczyk-krzysztof kowalczyk-krzysztof removed the request for review from a team November 22, 2024 09:12
@andreadelrio andreadelrio self-requested a review November 22, 2024 19:29
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Hi @kowalczyk-krzysztof, thanks for taking this on. After revising the design proposed last year I have an updated proposal. See:
Frame 508 (proposed nov 2024)

Essentially I think we should increase the font size and reduce the font weight, this will help with readability and reduces visual noise. Also, instead of using the warning EuiBadge we should use the hollow EuiBadge (I checked with Graham and he's onboard).

Also can you add some padding above the Preview heading? Something like 16px or 24px should work (see the screen attached to see how it should look).

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as draft November 25, 2024 08:15
@kowalczyk-krzysztof
Copy link
Member Author

@andreadelrio
Thanks for the update. I've made changes to this PR to include your latest design.
updated-filters

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as ready for review November 25, 2024 14:51
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

@kowalczyk-krzysztof thanks for the updates. I'm still seeing some Preview text in 12px, please change to 14px.

@andreadelrio
Copy link
Contributor

@kowalczyk-krzysztof please see:
CleanShot 2024-11-25 at 13 37 02@2x

@kowalczyk-krzysztof
Copy link
Member Author

@kowalczyk-krzysztof please see: CleanShot 2024-11-25 at 13 37 02@2x

Thanks for the feedback @andreadelrio - I'll make the requested changes, however, when it comes to filter pill, I'm not sure if it'll be a quick fix as the component used in preview and filter pill is the same in both cases and I'm not sure yet if there's an easy way to differentiate the source.

cc: @ghudgins

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as draft November 26, 2024 09:43
@ghudgins
Copy link
Contributor

I caught up with @andreadelrio and think we can proceed with her recommendations even though it's going in both places

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as ready for review November 26, 2024 21:32
@elasticmachine
Copy link
Contributor

💚 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
unifiedSearch 357.5KB 358.1KB +688.0B

History

cc @kowalczyk-krzysztof

@kowalczyk-krzysztof
Copy link
Member Author

@ghudgins
Ok so if I understood correctly, we're going with the approach that both filter pills and filter preview will behave the same? If that's true, @andreadelrio could you please re-review this PR.

@ghudgins
Copy link
Contributor

yes, looks good to me. just checked it out on the branch.

@andreadelrio any concerns?

image

as it is today
image

@andreadelrio
Copy link
Contributor

andreadelrio commented Dec 2, 2024

yes, looks good to me. just checked it out on the branch.

@andreadelrio any concerns?

image as it is today image

The introduction of the badge inside the pill makes pills look slightly more chunky (see image attached). This is an issue because we're trying to make our UI elements compressed (already did with controls, Unified Search bar will follow). There's also the issue of it kind of looking like a filter pill is inside another filter pill which is not the case. @kowalczyk-krzysztof can you confirm whether the effort to keep the styles separate be too big?

image

@kowalczyk-krzysztof
Copy link
Member Author

yes, looks good to me. just checked it out on the branch.
@andreadelrio any concerns?
image
as it is today image

The introduction of the badge inside the pill makes pills look slightly more chunky (see image attached). This is an issue because we're trying to make our UI elements compressed (already did with controls, Unified Search bar will follow). There's also the issue of it kind of looking like a filter pill is inside another filter pill which is not the case. @kowalczyk-krzysztof can you confirm whether the effort to keep the styles separate be too big?
image

@andreadelrio Yeah I couldn't really figure out a way to properly separate the styles and I still don't think it's a quick change and the level of effort required might be too big, considering this was supposed to be a papercut. The easiest solution would be just to give up on the badge and keep it as text in both places.
cc @ghudgins

@ghudgins
Copy link
Contributor

ghudgins commented Dec 3, 2024

if there's nothing we can do about the vertical space we're creating in the pills then yeah we should close the PR. sorry for the runaround....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Unified search Unified search related tasks release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Unified Search] Improve Preview UX for long field names in filter modal
5 participants