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

feat(discover): Use URL parameters for each visualization #11428

Closed
wants to merge 2 commits into from

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Jan 9, 2019

Instead of using component state, use url parameters as data source for which chart gets displayed. This will be helpful when navigating from other parts of the application to a discover query.

Instead of using component state, use url parameters as data source for which chart gets displayed. This will be helpful when navigating from other parts of the application to a discover query.
@billyvg billyvg force-pushed the feat/discover/use-links-for-visualizations branch from 0b139d0 to 228e5e6 Compare January 9, 2019 01:17
.at(1)
.simulate('click');
.simulate('click', {button: 0});
Copy link
Member Author

Choose a reason for hiding this comment

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

react-router checks for this

@billyvg billyvg requested review from lynnagara and a team January 9, 2019 01:20
@lynnagara
Copy link
Member

lynnagara commented Jan 9, 2019

This seems to break toggling in discover. I'd also suggest dropping this keepVisual option and just doing this by default. I think it'd be good to move some of this code into a utility function that validates the visualization requested against the query rather than in the main discover.jsx component

@billyvg
Copy link
Member Author

billyvg commented Jan 10, 2019

@lynnagara Ah this is completely broken - I initially was comparing location.query instead of location.search in cWRP but it broke all the tests because they only updated search. I think manipulating query makes it a bit easier when removing/clearing query params.

The problem with always keeping visual is when you're in a by day chart and you edit your query so that it doesn't support grouping by time (e.g. can only render a Table result), how do you suggest we get around that?

@billyvg
Copy link
Member Author

billyvg commented Jan 10, 2019

Updated: 6a44b31

@lynnagara
Copy link
Member

Here's an example of how I think the persistent querystring can work - i don't think it's significantly more complicated #11467

@billyvg billyvg closed this Jan 11, 2019
@evanpurkhiser evanpurkhiser deleted the feat/discover/use-links-for-visualizations branch March 27, 2019 18:21
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants