-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiIcon] Machine Learning icons for panels #7873
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.
These new icons look great, @joana-cps! I left a few small comments for your review (and one question for the EUI team). Once those are addressed, I think this will be good to go.
Other than that, in case you haven't already, do feel free to update the Figma EUI library with these new icons so all designers have the ability to use them. Thanks!
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.
@elastic/eui-team: Do ya'll have a preference on how we should be naming our SVGs? Should we use underscores or camel casing? Currently we use both. I've always tended to use camel casing because it made things easier in the icon map file. Then again, excluding SVGs, it looks like all other files use underscores. Thoughts?
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.
We should keep the snake case (underscore) file naming convention whenever possible. The final icon key in icon_map.ts
should use camel case and map to the snake-cased file name, e.g., arrowDown: 'arrow_down'
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, @tkajtoch. I'll make sure to do that going forward. Would it be worth opening a separate issue to update all the SVG file names and mappings to reflect this for consistency?
packages/eui/src/components/icon/svgs/change_point_detection.svg
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Marcialis <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
Co-authored-by: Michael Marcialis <[email protected]>
Thanks @MichaelMarcialis, all comments are reviewed on my side now. |
💚 Build Succeeded
History
|
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 making those changes, @joana-cps. LGTM!
`v95.3.0` ⏩ `v95.4.0` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v95.4.0`](https://github.com/elastic/eui/releases/v95.4.0) - Added `anomalyChart`, `anomalySwimLane`, `changePointDetection`, `fieldStatistics`, `logPatternAnalysis`, `logRateAnalysis` and `singleMetricViewer` glyph to `EuiIcon` ([#7873](elastic/eui#7873)) **Bug fixes** - Fixed overlapping content in `EuiBasicTable` for expanded and selectable table rows ([#7895](elastic/eui#7895)) - Fixed the alignment of `EuiBasicTable` mobile actions ([#7895](elastic/eui#7895)) **Accessibility** - Improved `EuiStat`'s screen reader accessibility ([#7864](elastic/eui#7864)) --- ## Additional Changes - reverts temporary fix for overlapping content in nested tables done in PR [#188374](#188374) --------- Co-authored-by: Elastic Machine <[email protected]>
Summary
Set of 7 new icons for Machine Learning panels.
Machine Learning panels, available in Dashboards, didn't have a designated icon. With the upcoming change in the add panel flow in Dashboards it became mandatory for consistency with the other panels.
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@default
if default values are missing) and playground togglesIf applicable, added the breaking change issue label (and filled out the breaking change checklist)