Skip to content
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

Add default values for filter labels property for xy charts #38644

Merged
merged 2 commits into from
Jun 17, 2019

Conversation

flash1293
Copy link
Contributor

Summary

Related to #30138 (comment) and #21589
This PR sets the currently undefined filter prop 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 AxisConfig be somehow reflected back to the UI? At the moment there is no such "back channel" - config args are passed down from the angular form to AxisConfig and 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 of AxisConfig seems 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.js stating // _.defaultsDeep mutates axisConfigArgs nested values so we clone it first.

Checklist

Use strikethroughs to 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

For maintainers

@flash1293 flash1293 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jun 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@flash1293
Copy link
Contributor Author

Jenkins, test this. Failure looks unrelated.

@flash1293 flash1293 marked this pull request as ready for review June 17, 2019 15:29
@flash1293
Copy link
Contributor Author

@ppisljar Thanks for the explanation 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants