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: remove timeScale update from fingerprint details link from insight details #98143

Merged

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Mar 7, 2023

Part of #97952.

This PR removes the setTimeScale call from the StatementDetailsLink
and TransactionDetailsLink functions. Navigating to the Fingerprint Details
pages from the Insights Details pages will no longer set the global time scale.

Loom (CC Console): https://www.loom.com/share/ca388c8c0068446d960f89cc6c1f6af5
Loom (DB Console): https://www.loom.com/share/209f2d21a1784d1d86adffabddb9b47f

Epic: none

Release note: None

@ericharmeling ericharmeling requested review from a team March 7, 2023 15:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@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.

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)


-- commits line 4 at r1:
I would change to "part of" because you won't be able to backport this and the change is different for 22.2, so once you have that in it can be consider "fixed"

Also, the same thing happens on Transaction Insights, so this change should also happen there.

Can you confirm that clicking on fingerprint id from the insights overview page is also working (not just clicking from the insights details page)

…t details

Part of cockroachdb#97952.

This commit removes the setTimeScale call from the StatementDetailsLink
and TransactionDetailsLink functions. Navigating to the Fingerprint Details
pages from the Insights Details pages will no longer set the global time scale.

Epic: none

Release note: None
@ericharmeling ericharmeling force-pushed the rm-stmt-details-link-ts-update branch from 4b95c34 to e3ceb51 Compare March 7, 2023 17:04
@ericharmeling ericharmeling changed the title ui: remove timeScale update from statement details link from insight details ui: remove timeScale update from fingerprint details link from insight details Mar 7, 2023
Copy link
Contributor Author

@ericharmeling ericharmeling 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 @maryliag)


-- commits line 4 at r1:

I would change to "part of" because you won't be able to backport this and the change is different for 22.2, so once you have that in it can be consider "fixed"

Done.

Also, the same thing happens on Transaction Insights, so this change should also happen there.

Done.

Can you confirm that clicking on fingerprint id from the insights overview page is also working (not just clicking from the insights details page)

Done. Updated the loom videos.

Copy link
Contributor

@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.

Nice! :lgtm:

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

@ericharmeling
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 7, 2023

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