Add default values for filter labels property for xy charts#38644
Add default values for filter labels property for xy charts#38644flash1293 merged 2 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-app |
💔 Build Failed |
ppisljar
left a comment
There was a problem hiding this comment.
the defaults from vislib should not propagate back up imo.
vislib is a charting library, it allows you to create charts based on provided configuration and data. with configuration it does apply certain defaults so you don't have to set all parameters manually.
vislib_vis_types is kibana plugin which registers a few kibana visualizations. with each visualization we provide the editor. each visualization should also provide default settings for anything that can be set in the editor. in case of vislib_vis_types we use vislib internally as a charting library. However we don't expose every vislib setting to the ui. Also some ui settings might affect multiple setitings in vislib charting library.
code in this PR does the right thing, adds a missing default to vislib vis types.
|
Jenkins, test this. Failure looks unrelated. |
|
@ppisljar Thanks for the explanation 👍 |
💔 Build Failed |
💚 Build Succeeded |
Summary
Related to #30138 (comment) and #21589
This PR sets the currently undefined
filterprop for the labels configuration for xy charts to the value which is actually used on the axis config (src/legacy/ui/public/vislib/lib/axis/axis_config.js:79).It solves this particular bug but I'm very unsure about the data flow in this case - shouldn't the default values from
AxisConfigbe somehow reflected back to the UI? At the moment there is no such "back channel" - config args are passed down from the angular form toAxisConfigand mixed with defaults there, but the applied defaults never flow back to the form. Trying to keep the pre-sets in the files changed in this PR in sync with the default behavior ofAxisConfigseems very brittle and error prone. Plus it doesn't solve the issue for already saved visualizations (#21589 (comment)).What do you think, @ppisljar ? Should we enable a way to reflect the defaults from the axis config back to the angular form? I didn't implement something like this because there is an explicit comment from you in
src/legacy/ui/public/vislib/lib/axis/axis_config.jsstating// _.defaultsDeep mutates axisConfigArgs nested values so we clone it first.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