[EuiIcon] Index glyph #7498
[EuiIcon] Index glyph #7498
Conversation
|
How do you intend to use this icon? I'm wondering is if we're using other icons elsewhere in Kibana to represent indexes already. |
|
In this PR elastic/kibana#175473 you have some use cases were using the search bar on top it will list also indices as a results cc: @julianrosado |
|
@JasonStoltz I typically use |
|
My concern is that we now have 2 ways of representing indices with icons. My question is, with the addition of this icon, which of the 2 icons should we use going forward? If it is this new icon, should we update our usage elsewhere? We should be consistent. FWIW, I have no objections to the addition of this icon and I don't mean to hold you all up. This is something we can address later. @MichaelMarcialis is assigned as the approver so I'm going to wait for him to take a look before pressing approve. |
|
My vote would be the new icon, since it's consistent with the other index manipulating icons (save for index flush, which is a weird outlier). |
|
That makes sense. Also, I didn't realize how old these index icons were. It looks like they've been around for at least 7 years -- so not necessarily a new problem! ce1c150#diff-cc02021d64f5f8e1da60f97c9e714874f263ca7a5088c8bd0e5f40343f0ec204 |
|
Hi @JoseLuisGJ, Thank you for creating the index icon. I am fairly new in here so wondering if I have to wait till this PR gets merge to import this icon into my PR. If yes, any idea when it might get approved? |
There was a problem hiding this comment.
Thanks for putting this together, @JoseLuisGJ! The only thing I have to mention is that it looks like the icon itself doesn't sit perfectly on the pixel grid, due to it being vertically centered in it's frame in Figma. While this won't have an effect on high pixel density displays, regular displays will cause parts of the icon to appear blurry (due to sub-pixel coloring). I've adjusted the icon in the Figma EUI library so it rests back on the pixel grid. Would you mind updating your SVG here with that change?
Otherwise, this looks good-to-go from my perspective. Approving now so I don't hold you up further.
|
Thanks for the fix @MichaelMarcialis , I committed ed152df the icon with the one you adjusted to the pixel grid. |
|
Hi @JoseLuisGJ, Thank you for being on top of it. I checked the doc site and it looks perfect. Hi @JasonStoltz and @cee-chen, any approx timeline when this will be available to import into my PR? I am trying to push my changes before the feature freeze and knowing the timeline will definitely help me to communicate with my PM. |
There was a problem hiding this comment.
Thanks for updating the icon, @JoseLuisGJ. I left one last comment below. Once it's addressed, I think this PR is good to go from my perspective. Approving now so I don't hold you up.
src/components/icon/svgs/index.svg
Outdated
| @@ -0,0 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" width="17" height="16" viewBox="0 0 17 16"> | |||
There was a problem hiding this comment.
Looks like the SVG may have been exported using an incorrectly sized frame. The width here is 17 when it should be 16 in both the width and viewBox attributes. Would you mind correcting? Thanks!
There was a problem hiding this comment.
So crazy with Figma. It has 16 x 16 in Figma and exports it with 17 and 16. Even already existing icons have the same issue. Anyway fix it to width="16" and viewBox="0 0 16 16"
There was a problem hiding this comment.
It does that all the time and I've never found a solution either.
|
@JasonStoltz Did you have any further pattern-related thoughts/objections to this or are we good to merge this in? |
|
@cee-chen Nope, let's merge it |
src/components/icon/assets/index.tsx
Outdated
There was a problem hiding this comment.
Adding here as a quick note - index.tsx is technically a 'special' filename as far as webpack is concerned (considered, e.g., the root of a folder). If it's not breaking any existing behavior it's probably fine, just wanted to note that since it might be a tad confusing to other devs
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
`v93.0.0` ⏩ `v93.1.1` --- ## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0) **This is a patch release primarily intended for use by Kibana.** - Added top-level `EuiTreeView.Item` export ([#7526](elastic/eui#7526)) ## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0) - Added `index` glyph to `EuiIcon` ([#7498](elastic/eui#7498)) - Updated `EuiHighlight` to accept an array of `search` strings, which allows highlighting multiple, separate words within its children. This new type and behavior *only* works if `highlightAll` is also set to true. ([#7496](elastic/eui#7496)) - Updated `EuiContextMenu` with a new `panels.items.renderItem` property, which allows rendering completely custom items next to standard `EuiContextMenuItem` objects ([#7510](elastic/eui#7510)) - `EuiSuperDatePicker` updates: - Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([#7501](elastic/eui#7501)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `intervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([#7501](elastic/eui#7501)) - Updated `onRefreshChange` to pass back a new `intervalUnits` key that contains the current interval unit format (seconds, minutes, or hours). ([#7501](elastic/eui#7501)) - Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop, which defaults to true (current behavior). To preserve displaying the unit that users select for relative time, set this to false. ([#7502](elastic/eui#7502)) - Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop, which accepts a minimum number in milliseconds ([#7516](elastic/eui#7516)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `minInterval` prop, which accepts a minimum number in milliseconds ([#7516](elastic/eui#7516)) **Bug fixes** - Fixed `EuiHighlight` to not parse `search` strings as regexes ([#7496](elastic/eui#7496)) - Fixed `EuiSuperDatePicker` submit bug when used within `<form>` elements ([#7504](elastic/eui#7504)) - Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to items without expandable children ([#7513](elastic/eui#7513)) **CSS-in-JS conversions** - Converted `EuiTreeView` to Emotion. Updates as part of the conversion: ([#7513](elastic/eui#7513)) - Removed `.euiTreeView__wrapper` div node - Enforced consistent `icon` size based on `display` size
`v93.0.0` ⏩ `v93.1.1` --- ## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0) **This is a patch release primarily intended for use by Kibana.** - Added top-level `EuiTreeView.Item` export ([elastic#7526](elastic/eui#7526)) ## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0) - Added `index` glyph to `EuiIcon` ([elastic#7498](elastic/eui#7498)) - Updated `EuiHighlight` to accept an array of `search` strings, which allows highlighting multiple, separate words within its children. This new type and behavior *only* works if `highlightAll` is also set to true. ([elastic#7496](elastic/eui#7496)) - Updated `EuiContextMenu` with a new `panels.items.renderItem` property, which allows rendering completely custom items next to standard `EuiContextMenuItem` objects ([elastic#7510](elastic/eui#7510)) - `EuiSuperDatePicker` updates: - Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([elastic#7501](elastic/eui#7501)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `intervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([elastic#7501](elastic/eui#7501)) - Updated `onRefreshChange` to pass back a new `intervalUnits` key that contains the current interval unit format (seconds, minutes, or hours). ([elastic#7501](elastic/eui#7501)) - Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop, which defaults to true (current behavior). To preserve displaying the unit that users select for relative time, set this to false. ([elastic#7502](elastic/eui#7502)) - Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop, which accepts a minimum number in milliseconds ([elastic#7516](elastic/eui#7516)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `minInterval` prop, which accepts a minimum number in milliseconds ([elastic#7516](elastic/eui#7516)) **Bug fixes** - Fixed `EuiHighlight` to not parse `search` strings as regexes ([elastic#7496](elastic/eui#7496)) - Fixed `EuiSuperDatePicker` submit bug when used within `<form>` elements ([elastic#7504](elastic/eui#7504)) - Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to items without expandable children ([elastic#7513](elastic/eui#7513)) **CSS-in-JS conversions** - Converted `EuiTreeView` to Emotion. Updates as part of the conversion: ([elastic#7513](elastic/eui#7513)) - Removed `.euiTreeView__wrapper` div node - Enforced consistent `icon` size based on `display` size
Summary
Adding a base index icon using same designs we have for its variants but we didn't for the root element.
General checklist