[Lens] Adds dynamic table cell coloring #95217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@elasticmachine merge upstream |
…a into lens/dynamic-coloring-table
|
@elasticmachine merge upstream |
mbondyra
reviewed
May 27, 2021
|
|
||
| return ( | ||
| <EuiFlexItem | ||
| key={id} |
There was a problem hiding this comment.
There's still something not workiing completely right - I think it's because the id is not persisted between rerenders.
What happens is that when I type a number the focus is being lost IF the number change won't cause reordering.
Try for example with ranges [20,40,60,80] to change 20 to 2 and then try to add zero again.
Same happens when you changed the colors - it closes on debounce instead of keeping it open.
flash1293
reviewed
May 27, 2021
There was a problem hiding this comment.
This looks almost good to merge for me.
Apart from the issue with temporary ids Marta found the current logic seems to break a bit if no data is rendered and changes are made:
It's an edge case - I suggest to just default to 0 - 100 in that case because we can't really do it "right" and it shouldn't happen often.
|
Both issues should have been addressed now. As for the id references I've used first a positional cache for the ids, but then restored a plain id generation. |
|
Now the values don't update when switching between percentage and number:
Should we take a step back here and try to stabilize this logic a bit? |
wylieconlon
approved these changes
May 27, 2021
There was a problem hiding this comment.
Other than the issues that @flash1293 and @mbondyra have already commented on, I wasn't able to find anything new. I did see that the error when there is no active data has turned into a console warning only, saying The specified value "NaN" cannot be parsed. It does not appear to break anything, but it would be nice if we could avoid this case.
|
Looks great to me except for the empty data case.
|
|
The previous fix was operation at the wrong level. Fixed the common issue at the root level, so now it should be consistent and no more warnings should be shown in the console. |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/anomaly_detection/saved_search_job·ts.machine learning anomaly detection saved search with filter and kuery query job creation loads the multi metric wizard for the source dataStandard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/anomaly_detection/saved_search_job·ts.machine learning anomaly detection saved search "after all" hook in "saved search"Standard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml.machine learning "after all" hook in ""Standard OutStack TraceMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
flash1293
approved these changes
May 28, 2021
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
May 28, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Michael Marcialis <michael@marcial.is>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
May 28, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Michael Marcialis <michael@marcial.is> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Michael Marcialis <michael@marcial.is>
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
May 28, 2021
* master: (77 commits) [RAC][Security Solution] Register Security Detection Rules with Rule Registry (elastic#96015) [Enterprise Search] Log warning for Kibana/EntSearch version mismatches (elastic#100809) updating the saved objects test to include more saved object types (elastic#100828) [ML] Fix categorization job view examples link when datafeed uses multiple indices (elastic#100789) Fixing ES archive mapping failure (elastic#100835) Fix bug with Observability > APM header navigation (elastic#100845) [Security Solution][Endpoint] Add event filters summary card to the fleet endpoint tab (elastic#100668) [Actions] Taking space id into account when creating email footer link (elastic#100734) Ensure comments on parameters in arrow functions are captured in the docs and ci metrics. (elastic#100823) [Security Solution] Improve find rule and find rule status route performance (elastic#99678) [DOCS] Adds video to introduction (elastic#100906) [Fleet] Improve combo box for fleet settings (elastic#100603) [Security Solution][Endpoint] Endpoint generator and data loader support for Host Isolation (elastic#100813) [DOCS] Adds Lens video (elastic#100898) [TSVB] [Table tab] Fix "Math" aggregation (elastic#100765) chore(NA): moving @kbn/io-ts-utils into bazel (elastic#100810) [Alerting] Adding feature flag for enabling/disabling rule import and export (elastic#100718) [TSVB] Fix Upgrading from 7.12.1 to 7.13.0 breaks TSVB (elastic#100864) [Lens] Adds dynamic table cell coloring (elastic#95217) [Security Solution][Endpoint] Do not display searchbar in security-trusted apps if there are no items (elastic#100853) ...
This was referenced Jun 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes: #66192, #68428, #96401, #96402
This PR adds the dynamic cell value coloring to the Lens application.
Iteration 2 completed:
color_stops) and shared logic to reuse for other featuresIteration 1
Plan: * Lens * [x] Enable a palette editor for numeric dimensions in the Datatable visualization * [x] `None` | `Cell` | `Text` options * [x] Create a panel to edit predefined palettes and ranges * [x] Filter out palette with no dynamic range * [x] Absolute and percentage ranges feature * [x] Auto range * [x] Reverse palette feature * [x] Steps input (not in the initial design) * [x] Range min/max validation * [x] `Gradient`/`Fixed` options * [x] Create a panel to create new custom palettes * [x] Add new `Stepped` option (pending #96459) * [x] Color stops editor (default to 3 stops to start with) * [x] Carry last predefined palette to `Custom` editor * [x] Reverse palette feature * [x] Absolute and percentage ranges feature * [x] Auto range * [x] High contrast computation for `Cell` dynamic coloring * [x] Checked light/dark themes * [x] Removed unused `NativeRenderer` code in the existing `PalettePicker` component (as discussed offline) * [x] Implement flyout for Panel * [x] Add component/unit tests for dimension editor * [x] Add component/unit tests for datatable rendering * [x] Add a11y/functional tests * Palette Service/Definition * [x] Extended Palette definition with more parameters to handle ranges/custom stops * [x] Add tests for new parameters * [x] Extended `PaletteDefinition` interface with new `getGradientColorHelper` for dynamic gradient coloring * [x] Migrated previous `getColor(s)` to `getCategoricalColor(s)` * [x] Add tests for new method * [x] Added some documentation about custom palettes in LensLatest screenshots:
Reverse colors will also reverse the palette preview:
Reverse colors with custom palette:
Custom palette with percentage values (note stop thumbs values are synced with range):
Custom palette with
Auto define rangesets the stops to 0-100%:Stepped gradient with custom palette:
Tooltip on auto range set:
Palette indicator on dimension field when dynamic coloring is enabled:
Discussion
Colors are quite a big topic, and while many arguments in this topic have been planned and thought through already, there may be specific scenarios to address.
High contrast
I've used the
isDarkColorfrom the EUI toolkit, but I'm not super happy about the final result. Canvas has a proprietary solution which is more aggressive on the contrast check and provider, in my opinion, a nicer result.As for now I've left the EUI implementation which should be WACG compliant.
this applies for the
Cellcoloring mode (cell background).Text coloring is a bit more tricky and at the moment I'm looking for a solution for that. As shown in the pictures above, the Gray scale palette at the bottom right (text coloring) has some readability issue.
Palette Display vs Stops Editor mismatch
There's a mismatch in rendering
fixedpalettes between theDisplayandStopsEditorin EUI, where the former shifts left all the color segments. There's currently an open issue in the EUI repo: elastic/eui#4664This can be a problem, maybe solvable on the Lens side
(haven't tried yet): solved.Force clean the DataGrid style
The datagrid is colored using a
useEffecthook, as explained in the EUI documentation. There's a clean up issue which is currently handled with the returned function in theuseEffect.There's an open issue in the EUI repo for that: elastic/eui#4665
This is already handled on the Lens side with the forced cleanup.
Transposable columns
How should range be computed for Transposable columns?
This is probably a general "range" discussion about Transposable columns, but in this context, it would be nice to make it clear.
At the moment each column computes its own range and colors are associated in "column isolation".
I may see that in some context it would be nice to have an "overall" range computed for the dimension transposed.
Is the current decision ok? Should I open a new issue for the "overall" option?
Extended range coloring
At the moment the color range is effectively working as in the picture here, where the extreme color stops are "extended":If the green stop marks theminValue: 0, any negative number will still be colored as green. The same applies for any value above the violet mark.While this makes sense, a useful feature would be to not extends the extremes in some context, and only color within the given range. This is probably an enhancement: do you think it make sense to open an issue for it?
Note since @MichaelMarcialis feedback this has changed. Values out of range are now not colored anymore.Added continuity option to handle this.
Checklist
Delete any items that are not applicable to this PR.