-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix(sqllab): Removed the tooltip from CopyToClipboard button in sqllab #18749
fix(sqllab): Removed the tooltip from CopyToClipboard button in sqllab #18749
Conversation
> | ||
{this.getDecoratedCopyNode()} | ||
</Tooltip> | ||
this.getDecoratedCopyNode() |
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.
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.
Agreed, the solution should be prop-based. Either
- render the tooltip conditionally (if it was passed a meaningful title)
- provide a prop or other means (e.g. title={false}) to intentionally hide the tooltip
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.
I think this solves the issue in a much more simple/estensible way.
Two non-blocking things worth consideration:
- Minor, but: since the norm is that the tooltip is visible, should the (optional) prop be
hideTooltip
with a default offalse
? Just a philosophy question, it works either way. - The two blocks of JSX in
renderNotWrapped
andrenderLink
are nearly identical. Should we DRY that up while we're at it here?
Oh, it would also be nice if the new prop is reflected/togglable in the Storybook entry. |
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.
Sorry to keep asking for things, but... can we add an RTL (React Testing Library) test here to make sure it stays working with and without the prop?
Codecov Report
@@ Coverage Diff @@
## master #18749 +/- ##
==========================================
- Coverage 66.32% 66.32% -0.01%
==========================================
Files 1620 1620
Lines 63060 63089 +29
Branches 6368 6373 +5
==========================================
+ Hits 41823 41842 +19
- Misses 19579 19590 +11
+ Partials 1658 1657 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@rusackas Ephemeral environment spinning up at http://52.24.232.107:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Removed the tooltip from CopyToClipboard button in sqllab
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION