-
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
Use EuiTokens for ES field types #57911
Use EuiTokens for ES field types #57911
Conversation
# Conflicts: # src/plugins/kibana_react/public/field_icon/__snapshots__/field_icon.test.tsx.snap # src/plugins/kibana_react/public/field_icon/field_icon.tsx
# Conflicts: # x-pack/legacy/plugins/lens/public/indexpattern_plugin/datapanel.tsx
# Conflicts: # src/legacy/core_plugins/kibana/public/discover/np_ready/components/table/table_row.tsx # src/legacy/ui/public/directives/field_name/field_name.tsx
Yell when / if this is ready for review. Did a visual scan and it looks ok. Noticed some tiny things in the code. Also tried merging this into my discover branch and it seems to do OK, |
This is awesome! It would be great to incorporate these icons into the mappings editor at some point. I did a quick audit, and there are a few field types that are available in the mappings editor, but do not have a dedicated icon. Here is the list with a brief description, along with a link to the documentation.
|
Thank you @alisonelizabeth !! Some of these other field types are quite complicated and I think it will take some time to come up with a visual mapping for each (or groups of types). For this particular PR, since most of the UI's I'm tackling use the simple ones that already have mappings (otherwise they just show a But let me know if there are any in the list that absolutely need to be addressed ASAP and I'll work to incorporate those in here. But we'll definitely get some visual mappings figured out for your implementation in the mappings editor. |
The method no longer returns the correct color and EuiProgress doesn’t currently support all the colors from the token map. @cchaos will follow up with a PR to address the coloring of the field data charts. Right now they will fallback to pink for progress and default for charts.
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.
Maps lgtm! Really nice improvement. Looks great!
- code review
- tested locally in chrome
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.
Graph changes LGTM, also looked briefly through Discover and Lens and everything looks fine for me. Thanks a lot for this!
@elasticmachine merge upstream |
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.
Code LGTM, very nice change!
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.
Couple questions. Looked locally though and seems ok.
x-pack/legacy/plugins/lens/public/indexpattern_datasource/_field_item.scss
Outdated
Show resolved
Hide resolved
<EuiHighlight search={searchValue}>{option.label}</EuiHighlight> | ||
</span> | ||
<EuiFlexGroup className={contentClassName} gutterSize="s" alignItems="center"> | ||
<EuiFlexItem grow={null}> |
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.
Is the null on purpose here?
<EuiHighlight search={searchValue}>{option.label}</EuiHighlight> | ||
</span> | ||
<EuiFlexGroup className={contentClassName} gutterSize="s" alignItems="center"> | ||
<EuiFlexItem grow={null}> |
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.
Same here?
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.
These values aren't technically wrong, so I'm just going to allow them.
x-pack/legacy/plugins/lens/public/indexpattern_datasource/_field_item.scss
Outdated
Show resolved
Hide resolved
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.
Looks great!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…_improve-advanced-settings-save * commit '98aa1d2d4f974f72a9a5397b1b91f11509f6fb7a': [SIEM] [Case] Enable case by default. Snake to camel on UI (elastic#57936) [File upload] Update remaining File Upload dependencies for NP migration (elastic#58128) Use EuiTokens for ES field types (elastic#57911) Added UI support for the default action group for Alert Type Model (elastic#57603) force savedObject API consumers to define SO type explicitly (elastic#58022) Update dependency @elastic/charts to ^17.1.1 (elastic#57634)
* [FieldIcon] Refactor to extend EuiToken and props * [Add to FieldIcon] Export props * [Graph] Updated instances of FieldIcon * [Maps] Update FieldIcon instance * [Lens] Update FieldIcon usage * [Discover] Update FieldIcon usage * Remove comment * [Lens] Delete unused files * [Graph] Fix alignments * More cleanup * Fixing snaps * [Lens] Removing method `getColorForDataType` The method no longer returns the correct color and EuiProgress doesn’t currently support all the colors from the token map. @cchaos will follow up with a PR to address the coloring of the field data charts. Right now they will fallback to pink for progress and default for charts. * [Maps] Fixing implementations of FieldIcon * [Graph] Using css utility class instead of custom class * Snap * Fix css to use var
…-out-of-legacy * 'master' of github.com:elastic/kibana: [SIEM] [Case] Enable case by default. Snake to camel on UI (elastic#57936) [File upload] Update remaining File Upload dependencies for NP migration (elastic#58128) Use EuiTokens for ES field types (elastic#57911) Added UI support for the default action group for Alert Type Model (elastic#57603) force savedObject API consumers to define SO type explicitly (elastic#58022) Update dependency @elastic/charts to ^17.1.1 (elastic#57634) [Endpoint] Add a flyout to alert list. (elastic#57926) Make sure index pattern has fields before parsing (elastic#58242) Sanitize workpad before sending to API (elastic#57704) [ML] Transform: Support multi-line JSON notation in advanced editor (elastic#58015) [Endpoint] Refactor Management List Tests (elastic#58148) [kbn/optimizer] include bootstrap cache key in optimizer cache key (elastic#58176) Do not refresh color scale on each lookup (elastic#57792) Updating to @elastic/[email protected] (elastic#54662) Trigger context (elastic#57870) [ML] Transforms: Adds clone feature to transforms list. (elastic#57837) [ML] New Platform server shim: update fields service routes (elastic#58060) [Endpoint] EMT-184: change endpoints to metadata up and down the code base. (elastic#58038)
EUI 19.0 added EuiToken types that line up with ES field types so that we can use them as a consistent way to render the symbols associated with these field types.
This PR will assume to update the current displays to use EuiToken and if possible, the singular component provided by Kibana: FieldIcon.
FieldIcon extends EuiToken and all of its props. However, it no longer accepts the
useColor
prop and instead consumers can pass down whatever color they want, including 'gray'. It also uses thescripted
flag to make the tokenfill: 'dark'
to differentiate.Example of new usages in:
Discover
Graph
Lens
Note: The
getColorForDataType
method no longer returned the correct color and EuiProgress doesn’t currently support all the colors from the token map, so it was removed. @cchaos will follow up with a PR to address the coloring of the field data charts. Right now they will fallback to pink for progress and default for charts.Maps
Checklist
Delete any items that are not applicable to this PR.
IE11IE is brokenFor maintainers