-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Vis public UI LESS to SASS #25333
Vis public UI LESS to SASS #25333
Conversation
Pinging @elastic/kibana-app |
Pinging @elastic/kibana-design |
💔 Build Failed |
It looks unrelated.... trying again JIC. Jenkins, test this |
💔 Build Failed |
- Legend back to hard-coded width - Tooltip breaks-all on value
💔 Build Failed |
💔 Build Failed |
Another ES error @elastic/kibana-app team, I think this is ready for review. The CI failures are inconsistent and all 3 have passed at some point... |
💔 Build Failed |
💚 Build Succeeded |
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.
Did a code review. Looks good. Likely should have someone from the viz team do a functionality pass.
…ess-to-sass # Conflicts: # src/core_plugins/kibana/public/dashboard/_index.scss # x-pack/plugins/ml/public/explorer/_explorer.scss
@elastic/kibana-app or @elastic/kibana-visualizations teams, can you do a review please? |
💚 Build Succeeded |
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've checked the main functionality of vis.
Everything looks good (and absolutely more clean to me ❤️) except for the following points to fix before merging:
- legend doesn't correctly shows tooltip when the title is a long text
- top horizontal legend with long text shows cutted tooltip
- tooltip values/headers on chart elements are displayed in a tabular format but aligned at the bottom of each cell: it's better to align all the cell values at the top
- some tooltip text goes to a new line without taking care of words, displaying partial words and values.
- missing the background of partial data areas: when displaying partial data (when the current selected time windows is smaller cut partially the date histogram interval at the beginning of end of a chart) we display a grayed zone with a tooltip that define that in this interval there is some partial data. Now this zone is no more shown.
I've also merged this #23806
Thank you @markov00 for your thoroughness. I've addressed most of the issues you mentioned except for a few:
I'm not getting that issue. Can you supply more info on your setup?
I removed the max-width on the item itself so this doesn't happen in
Fixed
Fixed
Fixed: I accidentally had both a low opacity and low rgba alpha value that was causing it to be very light. |
I've seen that also having the legend on the left shows an overflowing tooltip. Do you think it's possible to increase the tooltip z-index ? |
I found the answer my self about the tooltip: no... |
Yeah, I tried looking into it before and there didn't seem to be an easy way because of the way the components were separated. Thanks. So is that an approval? :) |
💚 Build Succeeded |
This PR removes the LESS files in ui/public/vis, visLib, visualize and replaces them with Sass.
Process taken
ui/public/_index.scss
file was updated include a_index.scss
file from each of the folders._index.scss
and_component_names.scss
files next to the components or views they live next to (or close to it).vis
was added to all selectors to namespace them and avoid conflicts.Notes
Darkmode
.. is still a bit hacky and re-imports the individual
_index.scss
files that are appropriate for.tab-dashboard.theme-dark
so you will see duplicate non-color styles in dark dashboard mode until we can finalize theming.Errors
I couldn't figure out how to get every type of error to show up inside the visualizations. If you can, please double-check that these are ok too.
Fixes
This fixes #22597, in the SASS way. There is currently a PR #23806 to fix it in the LESS files. If this fix needs a back port to before 6.6, I'd suggest merging that one in first.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately