-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] add new metric visualization #136567
[Lens] add new metric visualization #136567
Conversation
…kibana into 134242/new-metrics-vis-lens
The split that you and @flash1293 have established sounds good to me.
Ah, yes. I'm recalling that now. During these 8.5 discussions, I'd love to also discuss changing the min/max input placeholders and button icons to infinity symbols in those cases where we're using infinity instead of the min/max data set values (just so it's crystal clear to our users). |
…kibana into 134242/new-metrics-vis-lens
x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx
Outdated
Show resolved
Hide resolved
@mbondyra Colors should be fixed |
|
||
const togglePalette = () => setIsPaletteOpen(!isPaletteOpen); | ||
|
||
const idPrefix = htmlIdGenerator()(); |
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: moving id to outside scope of the component makes the id more stable, otherwise the id gets changed with every rerender - I didn't notice any behaviour it would impact, but it's probably not a good practice, especially because the name stays the same:
id.mov
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 definitely move the ID up the scope, but it seems like the name staying the same is an EUI bug?
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 the majority of those requested changes to quickly, @andrewctate! Amazing work! Acknowledging that we're pushing some comments/disucssions off to 8.5, this is looking good from my perspective. Adding a few last minute thoughts below, but approving now so I don't hold you up further. Thanks again for all your hard work on this!
- Within the "Metric" layer panel, the "Primary metric" dimension correctly shows a color palette preview when "Dynamic" color is selected. However, if "Static" color is selected, a color swatch preview does not appear to indicate the applied color for that dimension. Would it be possible to add one similar to how we do with other visualizations?
- Currently, the default "Primary metric" color of white (#ffffff) is being represented by a transparent swatch in the color picker, rather than a white swatch. Would it be possible to fix this?
- I know that @gvnmagni requested that we use white as the default background color for visualizations with only the "Primary metric" dimension populated. I understand the desire to do so in order to present ultimate neutrality and a blank slate for the users to build/customize upon. Currently however, I don't think we're in a good position to do so. I worry that users seeing the workspace as shown in the screenshot below might give the false impression that something is broken or wrong with their configuration. I know we're currently starting to have conversations around whether we should resize the workspace panel to conform to the visualization and/or whether we should provide perimeter borders for visualizations such as this, but I don't think the outcome of those discussions are planned to be added for 8.4. As such, I'm wondering if it makes better sense to restore the previous light gray default background color for visualizations with only the "Primary metric" dimension populated, just so it's clear to the user that nothing is broken. Thoughts?
- Do ya'll think we should change the "Progress bar direction" label to read "Bar direction" instead? Semantically, I view this element as more of a bar chart than a progress bar. This change would also help prevent the label from breaking a line.
- Do ya'll have any thoughts on changing the "Max columns" label to something like "Layout columns"? I'm wondering if this change helps to make it clearer as to what this setting does.
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 didn't find anything that should block this anymore and the other issues we agreed to push. Approved! 🥳
…kibana into 134242/new-metrics-vis-lens
} | ||
|
||
function MaximumEditor({ setState, state }: Props) { | ||
const idPrefix = htmlIdGenerator()(); |
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 as the comment below - maybe we can move it up the scope?
Fine by me! 👍 |
@MichaelMarcialis thank you! And thanks to @flash1293 and the team for pitching in to get this over the line.
Makes sense. Adding as a follow-up.
We changed it to "Auto." Does that solve it?
Done!
Done! |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Resolve #134242
This adds the new metric visualization.
Screenshots
Checklist
Delete any items that are not applicable to this PR.