-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: convert URLShortLinkButton to typescript #19954
chore: convert URLShortLinkButton to typescript #19954
Conversation
c6da7b0
to
c66bd11
Compare
Codecov Report
@@ Coverage Diff @@
## master #19954 +/- ##
=======================================
Coverage 66.28% 66.28%
=======================================
Files 1712 1712
Lines 63964 63964
Branches 6726 6726
=======================================
Hits 42396 42396
Misses 19856 19856
Partials 1712 1712
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much nicer now, thanks for cleaning this up! 👍 Minor improvement suggestion, other than that LGTM.
const { addDangerToast } = useToasts(); | ||
|
||
const getCopyUrl = async () => { | ||
if (dashboardId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got me wondering, can we really have this component without dashboardId
being populated: And based on a quick search, this appears to be the only component that's using this, and there dashboardId
is always set:
dashboardId: PropTypes.number, |
dashboardId
mandatory in URLShortLinkButtonProps
.
@villebro I made more changes to this PR:
|
filters={filters} | ||
showShortLinkButton | ||
/> | ||
<AnchorLink id={component.id} dashboardId={dashboardId} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding dashboardId
fixes the bug where the copy button doesn't copy the right URL for section headers in dashboards.
ad21c10
to
ee3141f
Compare
ee3141f
to
3304e31
Compare
Merging to unblock followup PRs |
SUMMARY
Convert URLShortLinkButton to TypeScript (tsx) in preparation for bigger refactor where we support including
activeTabs
in dashboard urls (currently you can only highlight one tab usinglocation.hash
).No visible changes in the UI.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION