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

[Graph] Fix default drilldown link on index pattern switch #34251

Merged
merged 2 commits into from
Apr 15, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Apr 1, 2019

Summary

Somehow fixes #18522 . The problem is that on selecting an index pattern a default drilldown template is created which contains the id of the selected index pattern. If the index pattern is switched afterwards, it is not updated.

This PR adds a flag isDefault to the auto-drilldown-template, which marks it to re-create it on index pattern switch. If the user edits the template, it is kept even when switching between index patterns. While it fixes the bug as long as the user didn't change anything, it might not always be the expected behavior (e.g. if the user just edits the icon of the drilldown).

A cleaner behavior would be removing all of the templates on index pattern switch, however this could result in data loss for the unexpecting user.

Another possibility would be making the default drilldown link readonly and non-removable, but this changes the current behavior of the UI and takes control from the user which I try to avoid.

What do you think, @timroes ?

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
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 added Feature:Graph Graph application feature Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Apr 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@flash1293 flash1293 requested a review from markharwood April 3, 2019 09:23
@flash1293
Copy link
Contributor Author

@markharwood What do you think about this? Would you say we can go with this solution for now?

@markharwood
Copy link
Contributor

What do you think about this? Would you say we can go with this solution for now?

Thanks for the fix. Seems like a reasonable approach.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor Author

@markharwood Could you approve the PR for merging?

Copy link
Contributor

@markharwood markharwood left a comment

Choose a reason for hiding this comment

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

Tried it out on master and works OK. LGTM

@markharwood markharwood self-requested a review April 11, 2019 14:05
@flash1293 flash1293 merged commit ed5ba0e into elastic:master Apr 15, 2019
@flash1293 flash1293 deleted the 18522/graph-default-drilldown branch April 15, 2019 07:45
flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Graph Graph application feature release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Graph] Drilldown links do not select the correct index pattern
3 participants