-
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
[Lens] Supports icon in the new metric #154210
Conversation
@@ -15,3 +15,23 @@ export const LabelPosition = { | |||
BOTTOM: 'bottom', | |||
TOP: 'top', | |||
} as const; | |||
|
|||
export const AvailableMetricIcons = { |
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.
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.
I was about to ask if we could add all icons from EUI but I realized how many are them, should we make a selection? How can we define the most important ones?
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.
Yes this is what we did for annotations and reference lines. We selected the icons we think are most commonly used. I dont know how we define the most important ones tbh. We can go with this set and change by user's feedback. Do you think they are important icons that are missing possibly and you can think of use cases?
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.
Probably better to use "pin" instead of "pinFilled" so that we are more consistent with other icons that are not filled, for the same reason I think we can avoid including "starFilled" if there are no other important reasons to have it since we already have "starEmpty"
From EUI icons, I would add:
- "globe", to have something related to geography, I'm thinking about having Metric related to countries or similar
- "temperature", to allow machines monitoring
From my point of view I don't see any other icons that might be important to have :)
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.
Thanx Gio, done 8681956
Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations) |
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.
Works well. Fun!
const getIcon = | ||
(type: string) => | ||
({ width, height, color }: { width: number; height: number; color: string }) => | ||
<EuiIcon type={type} width={width} height={height} fill={color} style={{ width, height }} />; |
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.
nit: this mapping isn't under test... not a big deal
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Closes #129229
Adds support for icon on the new metric.
Also it moves the IconSelect dropdown to the shared components (from XY) as it now reused from both visualizations.
Checklist