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: update url params when switching tabs on sql activity #71762

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

maryliag
Copy link
Contributor

When the user clicks on a tab on the new SQL Activity page
now the search params on the url is updated with the tab
selection.
This commit also create a common function to sync history
so all the pages can use it and remove duplicate code.

Release note: None

@maryliag maryliag requested a review from lindseyjin October 20, 2021 15:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

When the user clicks on a tab on the new SQL Activity page
now the search params on the url is updated with the tab
selection.
This commit also create a common function to sync history
so all the pages can use it and remove duplicate code.

Release note: None
Copy link
Contributor

@lindseyjin lindseyjin left a comment

Choose a reason for hiding this comment

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

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


pkg/ui/workspaces/cluster-ui/src/util/query/query.ts, line 74 at r3 (raw file):

  const nextSearchParams = new URLSearchParams(history.location.search);

  Object.entries(params).forEach(([key, value]) => {

Hi, I'm just a little bit confused about how this code works. Why do we need to delete keys in nextSearchParams if they're undefined in params? Is this because we want to keep old param values and only clear them if we explicitly pass in undefined values?

Copy link
Contributor

@lindseyjin lindseyjin left a comment

Choose a reason for hiding this comment

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

I tried running this locally, and everything works well!

One quick question though: I noticed that the sql activity page defaults to the Sessions tab, but 'tab=sessions' doesn't appear in the url when you first click into the page. Is that intentional/expected behaviour?

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

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! 0 of 0 LGTMs obtained (waiting on @lindseyjin)


pkg/ui/workspaces/cluster-ui/src/util/query/query.ts, line 74 at r3 (raw file):

Previously, lindseyjin (Lindsey Jin) wrote…

Hi, I'm just a little bit confused about how this code works. Why do we need to delete keys in nextSearchParams if they're undefined in params? Is this because we want to keep old param values and only clear them if we explicitly pass in undefined values?

Exactly, if a param is undefined we want to remove from here (no need to keep things in the url that don't add information), if we have a value we want to add/update this value.
For example, if you go to the statement page and do a search, it will add q=abc, then you filter based on app, this will add app=def so now you have q=abc&app=def, now you want to remove the search, for this on the Statement page we set the value of q to undefined and call this function, so now the params will be updated to have just app=def

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.

It's intentional, since the default value is session, just so we get a cleaner first url, since at that point it won't add any new information

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lindseyjin)

Copy link
Contributor

@lindseyjin lindseyjin left a comment

Choose a reason for hiding this comment

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

Thanks for clearing up my questions! LGTM :)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @lindseyjin)

@maryliag
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 20, 2021

Build succeeded:

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.

3 participants