fixes visualization editor selection being reset #30073
Conversation
ppisljar
added
WIP
|
Pinging @elastic/kibana-app |
💔 Build Failed |
ppisljar
force-pushed
the
fix/regionMap
branch
from
6ce6493 to
dfd1677
Compare
ppisljar
changed the title
💔 Build Failed |
ppisljar
force-pushed
the
fix/regionMap
branch
from
dfd1677 to
1f18d9f
Compare
💔 Build Failed |
There was a problem hiding this comment.
I tested this locally (Chrome OSX) and can confirm that it's working.
Just to make sure I understand the implementation: I assume the root cause of the issue was vis.params being mutated? (Presumably by decorateVisObject, or somewhere else too)?
What is the guidance going to be moving forward on the relationship between visParams vs vis.params, and when to use each? There are still lots of (unchanged) places where visualizations are referencing vis.params, and my only concern is that introducing another argument to the render() method of a vis still doesn't make its purpose immediately clear.
It seems to me that if we are trying to replace this method of accessing params, we should consider updating it everywhere (which could be a lot of places...). And perhaps even thinking of ways to discourage the use of vis.params so people don't shoot themselves in the foot moving forward.
|
@lukeelmers everything past visualizationLoader (so all visualizations) should not be using |
|
retest |
💔 Build Failed |
|
Any thoughts on test coverage? Functional looks tough given it's only there if you wait some time. But maybe possible to add unit/jest to cover it? |
|
the plan was to add a dummy unit test that would set a fake variable, run visualize loader and check if vis.params object wasn't mutated ... but a better idea would be well appreciated |
💔 Build Failed |
ppisljar
force-pushed
the
fix/regionMap
branch
from
1f18d9f to
174d8e9
Compare
💔 Build Failed |
ppisljar
force-pushed
the
fix/regionMap
branch
from
174d8e9 to
6245c16
Compare
💔 Build Failed |
💔 Build Failed |
ppisljar
force-pushed
the
fix/regionMap
branch
from
83baa89 to
7c13a81
Compare
💚 Build Succeeded |
# Conflicts: # src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
Okay, looked through this again and I think it all makes sense to me! Added one question.
One thing I think is worth thinking about as we move forward is whether there’s a good way to ensure people don’t accidentally use vis.params instead of visParams... it feels like a mistake that would be very easy to make.
|
@lukeelmers yes, the idea is to not pass down vis object anymore, but rather figure out what from it is still necessary and just pass down that specifically. Something to look into in the following weeks. |
| if (!(status.resize || status.data || status.params)) return; | ||
|
|
||
| if (status.params || status.aggs) { | ||
| this._updateParams(); | ||
| if (status.params || status.data) { |
There was a problem hiding this comment.
this._updateParams and this._updateData have the same if condition now and can be merged into one function since they are not called from somewhere else.
| visConfig.metric.metrics = schemas.metric; | ||
| metric: schemas => { | ||
| const visConfig = { dimensions: {} } as any; | ||
| visConfig.dimensions.metrics = schemas.metric; |
There was a problem hiding this comment.
Why is this put into dimensions now? Was this a bug in the old version? I realize this is the new pipeline infrastructure, just asking to learn about the code base.
There was a problem hiding this comment.
historically metric had a bit different structure of params that other visualizations (everything nested under metric). Initially i tried to follow that but realized its causing additional problems as then we have to check for that special condition in various places.
# Conflicts: # src/legacy/core_plugins/metrics/public/components/vis_editor.js
💔 Build Failed |
|
retest |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
# Conflicts: # src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts
Summary
Resolves #29881
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] 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