-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Update filtering icons #147724
Conversation
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.
LGTM 👍
Thanks for updating it, @andreadelrio !
@andreadelrio in ML we use the The files that would need editing I think are:
|
@peteharverson Great idea, I've updated those files. |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
The code changes here for ML look good, but I wonder if the icons are ok from an accessibility viewpoint? The plus / minus in the new designs are very small.
Previous design:
And inside the field popover in Discover for example we don't have a tooltip on the icons which would help clarify the actions.
@peteharverson We have checked them towards accessibility. Maybe @MichaelMarcialis and @andreadelrio can add a comment on how we did it. 👍 |
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.
Thanks for the confirmation on accessibility @ninoslavmiskovic . Would be interested to know how the checks were done @MichaelMarcialis @andreadelrio, but don't want to hold this up so LGTM for ML.
When analysing large datasets, it's common to view at higher data density, such as using chrome magnification at 80%. These icons feel too small and not an improvement. |
@MichaelMarcialis would have more context as to accessibility. I seem to remember we discussed making the icons bigger. Let's wait til he's back from PTO to continue this conversation. |
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.
can i fix the issue in discover grid
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.
can i fix the issue
Hey, gang! For some quick history, the issue that @ninoslavmiskovic, @andreadelrio, and myself tried to address here was our current over-use of the plus icons in our interfaces. As you can even see in the previous design screenshot shown above, the plus icon is used in the Discover field popover for two very different actions. Similar plus icon overuse exists in other areas of Kibana as well. To address that, we ultimately decided that making the include/exclude filter icons more obviously related to filtering would be the best way forward. After testing a number of icon variations and seeing how identifiable they were when scaled down, we selected the ones you see here (which were subsequently added to EUI). All that said, if there are concerns about the icons we landed on here, I'd be happy to chat about it further or make adjustments as needed. Just let us know! |
Summary
This PR updates the filter icons in Discover to use EUI's new
FilterInclude
andFilterExclude
. Closes #146802Changes include:
Data grid
Legacy document table
Expanded document flyout
Unified field list
popoverField Stats
I also updated one icon in the
Unified field list
popover fromplusInCircle
toplusInCircleFilled
for consistency. "Add field as column" usually usesplusInCircleFilled
.Field list popover
Expanded document
Data grid
Field stats
Legacy table
Checklist
Delete any items that are not applicable to this PR.
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listRisk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers