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

[sql-lab] only show the reset state button if location param present #1075

Merged
merged 2 commits into from
Sep 8, 2016
Merged

[sql-lab] only show the reset state button if location param present #1075

merged 2 commits into from
Sep 8, 2016

Conversation

ascott
Copy link
Contributor

@ascott ascott commented Sep 8, 2016

  • by default, hide the reset state button
  • to show reset button, add reset param: /caravel/sqllab?reset=1

screenshot 2016-09-07 18 13 24

screenshot 2016-09-07 18 13 32

plz review @mistercrunch @bkyryliuk @vera-liu

@@ -117,6 +117,7 @@ class SqlEditorTopToolbar extends React.Component {
}
render() {
const tables = this.props.tables.filter((t) => (t.queryEditorId === this.props.queryEditor.id));
const shouldShowReset = window.location.search === '?reset=1';
Copy link
Member

Choose a reason for hiding this comment

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

what if you have here other args?

Copy link
Member

Choose a reason for hiding this comment

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

I think we have a querystring parser function in the caravel module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sql-lab isn't using url params for anything else currently. i think this will probably be a short term thing, so it can be toggled for a demo tomorrow. we should remove it once most of the dev work is done.

@bkyryliuk
Copy link
Member

I am just curious, @ascott , do you know if there is a way in js to distinguish between dev and prod modes?

@mistercrunch
Copy link
Member

@bkyryliuk
Copy link
Member

thanks @mistercrunch great to know, looks like a simple check.
What do you think about using reset state button only in the dev mode?
@ascott , @mistercrunch ?

@ascott
Copy link
Contributor Author

ascott commented Sep 8, 2016

@mistercrunch do you want this for the demo today? is it useful for you if it's only available in dev?

@mistercrunch
Copy link
Member

mistercrunch commented Sep 8, 2016

It's good to have this button hidden somewhere if the state ever gets corrupt or incompatible with new code. We should do all we can to prevent any situation where clearing the state would fix issues, but it's good to have.

While developing I've gotten to situations where the state was incompatible and the "Reset State" button would not even render :( .

@ascott ascott merged commit 8eb4cbf into apache:master Sep 8, 2016
@ascott ascott deleted the alanna-toggle-reset-state branch September 8, 2016 20:54
ascott pushed a commit that referenced this pull request Sep 8, 2016
* only show the reset state button if location param (#1075)

* cherry pick commits from master
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants