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

ui: use push instead of replace on browser history on SQL Activity page #71938

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

maryliag
Copy link
Contributor

Previously, when changing tabs on SQL Activity page the history
would be replaced, this commits change to push to history, meaning
when the user clicks Back on the browser it will return for the
previous tab.

Release note: None

Previously, when changing tabs on SQL Activity page the history
would be replaced, this commits change to push to history, meaning
when the user clicks Back on the browser it will return for the
previous tab.

Release note: None
@maryliag maryliag requested a review from a team October 25, 2021 16:38
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

@maryliag
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 25, 2021

Build failed (retrying...):


const onTabChange = (tabId: string): void => {
setCurrentTab(tabId);
util.clearHistory(props.history);
util.syncHistory({ tab: tabId }, props.history);
props.history.location.search = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, history.location.search is already reset in line 83 in query.ts, do we need to reset that here as well ?

util.clearHistory(props.history);
util.syncHistory({ tab: tabId }, props.history);
props.history.location.search = "";
util.syncHistory({ tab: tabId }, props.history, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor nit: add a inline comment here for the boolean literal /* push */

Copy link
Contributor Author

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)


pkg/ui/workspaces/db-console/src/views/sqlActivity/sqlActivityPage.tsx, line 30 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

hmm, history.location.search is already reset in line 83 in query.ts, do we need to reset that here as well ?

yes, because the reset updates the current ones, but what I want is to remove all the query params before adding only the new tab selection

@craig
Copy link
Contributor

craig bot commented Oct 25, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Oct 25, 2021

Build succeeded:

@craig craig bot merged commit 7c50de2 into cockroachdb:master Oct 25, 2021
@maryliag maryliag deleted the use-push-history branch November 8, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants