feat: add autocopy functionality to CopyToClipboard (#10328) #13037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13037 +/- ##
===========================================
+ Coverage 53.38% 73.13% +19.74%
===========================================
Files 489 546 +57
Lines 17315 20183 +2868
Branches 4482 5275 +793
===========================================
+ Hits 9244 14761 +5517
+ Misses 8071 5297 -2774
- Partials 0 125 +125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| } else { | ||
| renderedChildren = Children.toArray(children); | ||
| } | ||
|
|
There was a problem hiding this comment.
| const renderedChildren = Children.toArray(element?.type == React.Fragment ? element.props.children : children); |
Not sure if it's easier to read in a one liner and/or with a null-coalescing operator. ¯\_(ツ)_/¯
|
This looks like some great engineering work @michael-s-molina , and I'm giving it a review. Meanwhile, just thinking about the feature, I have a small UX concern/curiosity for @zuzana-vej and/or @junlincc that I feel I should raise. Were flows around the email function considered, where the user might have something on the clipboard (e.g. the contents of an email) that gets nuked by this automatic copying? While it saves a click, I hope we're not frustrating other users even more. I'm wondering if that does potential UX harm by overwriting the contents of the clipboard when there's no clear indication that the "share" button will do so. I suspect that's why most sites (Youtube as one example) don't do this. If the intent of the change is simply to remove a click, then I wonder if Share should trigger a modal in the first place, which requires a click to close. The design of the Share button on Reddit, for example, serves the same purpose - open the menu, click "copy" and it just goes away unobtrusively. |
There was a problem hiding this comment.
Minor nit/question, but generally looks good to me from a code perspective, so I'm approving.
Before merging, I just want to make sure this is the approach we want to take on the UI/UX front in general though, as there may be other ways to save a click (or two) without surprising the user in an impactful way.
|
I share the same concern with @rusackas on the implicitness of the auto-copy behavior and how it may accidentally override users' existing clipboard. I'm wondering if we can replace the share button with a copy icon or move the share functionality (under two links, "Copy sharable URL" and "Share via Email") under the hamburger menu. I'd imagine the frustration mentioned in #10328 mostly come from the "slow" animation of the popover and the fact that you have to move your mouse horizontally to the left to find the Copy icon. Putting the copy link in a dropdown menu might make the action flow smoother. |
|
@rusackas Good point, haven't thought about that. There were few users who provided feedback that they need to copy twice just to share a link. Sharing a link is very common and action users perform many times even every day, while the share via email is not - but just because our users don't user it much doesn't mean others in community don't, and I can see how this could be confusing in general. @ktmud generally the feedback was that user needs 2 clicks, and that the copy button is small and user needs to navigate to it. Which on dashboard means moving mouse very far. On Chart Explore not but still user needs to move to the left to navigate to the small icon. Displaying the option to copy link RIGHT BELLOW the share link button (and then the email option below that) could be much better user experience (still 2 clicks, but much easier), which also addresses @rusackas's concern. The other option is to have two icons as @ktmud suggested too. |
|
+1 on this concern. I think there's a reason why the copy to clipboard is a separate action than share for almost all instances where this functionality appears. Wiping the user's clipboard and adding a new item is something that should only happen with the user's consent. I don't think "share" implies this consent. I think this could be solved by having the popover appear on hover or moving the copy clipboard button much closer to where the user mouse would be after taking the "share" action. @ktmud's solution also sound viable. In the case of dashboard, opening a modal does seem a bit heavy. Would we be open to revising that design to use a popover instead, or at least change to something that matches the other design? |
|
@michael-s-molina Thanks for the quick simulation! I think this works for the dashboard page. For the Explore page though, looking at this more, I'm now no so sure about replacing the icon as it's not very clear what does a copy icon mean. Most first-time users would probably associate it with copying the data, the query, or the chart metadata, not a short-url. Here's another proposal: wow about we keep the popover but make it open on hover and remove the animation? |
@ktmud I totally agree about the copy icon but I really like the idea to remove the popover and make this action more straightforward. Can't we just use the Fontawesome link icon? It's already being used in other copy buttons like this one:
I think the user will quickly assimilate, and also we have the tooltip to help. It would look like this:
or
What do you think? |
|
I think the link icon works, too, but let's make sure to add a tooltip "Copy chart URL to clipboard". It'll change the behavior of an existing icon, but maybe that won't be a big issue with the added tooltip? What do you all think? @evans @junlincc @nytai @zuzana-vej ? |
|
I think the 2nd image (link icon and email icon) is good. Only thing is that dashboard and explore have slightly different behavior (one has two icons and one has another menu item in the dropdown). But I think it's probably ok and I think it will save users time. In terms of the behavior (on Explore) changing, I think that's very straightfoward for users to get used to. |
junlincc
added
need:design-review
|
We have plan of moving those buttons and share under menu, leaving only |
michael-s-molina
force-pushed
the
autocopy-links
branch
from
0f40a2b to
fbe5420
Compare
|
In this last commit I did the following changes in addition to original changes:
I also created a React hook called |
|
This is so cool. Thanks for all the updates and the attention to details! @michael-s-molina |
|
@ktmud you wanna approve the code? |
junlincc
added
explore
| @@ -79,19 +79,19 @@ describe('HeaderActionsDropdown', () => { | |||
| expect(menu.find(SaveModal)).not.toExist(); | |||
| }); | |||
|
|
|||
| it('should render five Menu items', () => { | |||
| it('should render four Menu items', () => { | |||
There was a problem hiding this comment.
These count assertions can probably be moved into other tests so you don't have to keep track of whether the test title is consistent. But no need to update if this already works.
There was a problem hiding this comment.
I changed to "should render available Menu items" since this is nested in a describe with the role being tested. This way future changes are not going to impact this text.
| slice: PropTypes.object, | ||
| addDangerToast: PropTypes.func.isRequired, | ||
| addSuccessToast: PropTypes.func.isRequired, | ||
| }; |
There was a problem hiding this comment.
Next time, if we are to update propTypes, might as well just convert the whole component into tsx first.
You could also convert it to a functional component and use the useToasts hook.
There was a problem hiding this comment.
Updated to typescript!
| ...rest | ||
| } = props; | ||
|
|
||
| const getShortUrl = useUrlShortener(url); |
There was a problem hiding this comment.
I think you can just let useUrlShortener return shortUrl itself and add a loading state if shortUrl is falsy.
There was a problem hiding this comment.
The idea here was correct but the implementation was not. The idea is that useUrlShortener returns an async function to delay the POST request to the moment the user actually clicks the option. As an async function you can infer loading and error states. I fixed the code to reflect this. Previously the error route (when the server throws an error) was not functioning and now is fixed also.
| addSuccessToast(t('Copied to clipboard!')); | ||
| } catch (error) { | ||
| addDangerToast( | ||
| t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'), |
There was a problem hiding this comment.
I don't think there is a place to Ctrl + C from anymore after the updated UI.
There was a problem hiding this comment.
Great catch! Fixed.
|
|
||
| async function onShareByEmail() { | ||
| try { | ||
| const bodyWithLink = t('%s%s', emailBody, getShortUrl()); |
There was a problem hiding this comment.
t is probably not needed if this is just simple string concatenation.
There was a problem hiding this comment.
I checked that the parent component is responsible for the translation of email body. No need here. Removed.
Was just reviewing and testing this locally. 😃 |
|
@michael-s-molina @ktmud Thank you both for perfecting this change! this is way better than my original expectation! We will still need to revisit the layout in the near future, but sure to keep the essence from this PR. |
michael-s-molina
force-pushed
the
autocopy-links
branch
from
fbe5420 to
83c49b8
Compare
@ktmud Accepted you suggestion. Now we update the tooltip. |
…to one-click solution (apache#10328)
michael-s-molina
force-pushed
the
autocopy-links
branch
from
83c49b8 to
752e002
Compare
| try { | ||
| setCopyTooltip(t('Loading...')); | ||
| const shortUrl = await getShortUrl(); | ||
| await copyTextToClipboard(shortUrl); | ||
| setCopyTooltip(t('Copied to clipboard!')); | ||
| } catch (error) { | ||
| setCopyTooltip(t('Sorry, your browser does not support copying.')); | ||
| } |
There was a problem hiding this comment.
| try { | |
| setCopyTooltip(t('Loading...')); | |
| const shortUrl = await getShortUrl(); | |
| await copyTextToClipboard(shortUrl); | |
| setCopyTooltip(t('Copied to clipboard!')); | |
| } catch (error) { | |
| setCopyTooltip(t('Sorry, your browser does not support copying.')); | |
| } | |
| let shortUrl; | |
| try { | |
| setCopyTooltip(t('Loading...')); | |
| shortUrl = await getShortUrl(); | |
| } catch (error) { | |
| setCopyTooltip(t('ERROR: failed to generate chart URL.')); | |
| return; | |
| } | |
| try { | |
| await copyTextToClipboard(shortUrl); | |
| setCopyTooltip(t('Copied to clipboard!')); | |
| } catch (error) { | |
| setCopyTooltip(t('Sorry, your browser does not support copying.')); | |
| } |
I think we can use two try/catch to distinguish backend error from browser support error. For browsers that does not support copying, maybe you can also show the shortened URL in the tooltip (or an alert box, or a modal) so users can copy it themselves.
mistercrunch
added
🏷️ bot
SUMMARY
CopyToClipboard(Autocopy link when clicking link or share buttons on Dashboards and Charts #10328)copyTextToClipboardincopy.tsto be asynchronous and fixed promise typeI left the copy button in autocopy mode just in case the user navigates to another tab, copy some content, comes back to the application and wants to copy the link again without reopening the modal or popover.
Alert PR improves notification positioning with right and bottom margins helping the user to notice the message.
@junlincc @rusackas
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
1 - Go to a dashboard
2- Click on a chart 3 dots icon
3 - Select share chart
4 - The link should be automatically copied to clipboard
1 - Go to explore
2 - Click on chain icon in top right corner
3 - The link should be automatically copied to clipboard
ADDITIONAL INFORMATION