You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/09 19:39:10 UTC
[GitHub] [superset] michael-s-molina opened a new pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
michael-s-molina opened a new pull request #13037:
URL: https://github.com/apache/superset/pull/13037
### SUMMARY
- Adds autocopy functionality to `CopyToClipboard` (#10328)
- Fixes a bug where the copying was being called twice
- Changes `copyTextToClipboard` in `copy.ts` to be asynchronous and fixed promise type
- Adjusts the margin of buttons that contain an icon but no text
- Changes copy feedback to use notifications instead of tooltip changes
I 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](https://github.com/apache/superset/pull/12122) improves notification positioning with right and bottom margins helping the user to notice the message.
@junlincc @rusackas
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<img width="1773" alt="Screen Shot 2021-02-09 at 4 17 48 PM" src="https://user-images.githubusercontent.com/70410625/107416856-b8812280-6af3-11eb-898f-67e0ff62a3b3.png">
<img width="1782" alt="Screen Shot 2021-02-09 at 4 28 09 PM" src="https://user-images.githubusercontent.com/70410625/107416940-d77fb480-6af3-11eb-9e68-3e08a665f19b.png">
<img width="1772" alt="Screen Shot 2021-02-09 at 4 21 32 PM" src="https://user-images.githubusercontent.com/70410625/107416976-e0708600-6af3-11eb-9556-8151fe6a6cf8.png">
<img width="1787" alt="Screen Shot 2021-02-09 at 4 23 29 PM" src="https://user-images.githubusercontent.com/70410625/107416996-e6666700-6af3-11eb-84e8-7f9cb32385bb.png">
### 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
- [x] Has associated issue
- [x] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-780579786
> Code LGTM with a couple of small comments.
>
> A small UX suggestion: instead of using toast, you may also just updated the tooltip when copied was successful (you can even show a "Loading..." while fetching the shortened URL in the tooltip, too)
>
> <img alt="copied-to-clipboard" width="324" src="https://user-images.githubusercontent.com/335541/108005424-8c5f1900-6fad-11eb-9cb6-4700e3fb20b3.png">
Accepted you suggestion. Now we update the tooltip.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776642807
Thank you all for the valuable feedback. Nice catch @rusackas! I really like @ktmud's solution with fewer clicks and seamless integration with the interface. I even simulated this behavior bellow:
<img width="1775" alt="Screen Shot 2021-02-10 at 8 12 35 AM" src="https://user-images.githubusercontent.com/70410625/107503459-be6d1700-6b78-11eb-8c91-6cbc8645c447.png">
<img width="1770" alt="Screen Shot 2021-02-10 at 8 15 36 AM" src="https://user-images.githubusercontent.com/70410625/107503466-c2009e00-6b78-11eb-9470-c453eab3420d.png">
What do you all think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-777840169
We have plan of moving those buttons and share functionalities under menu, leaving only `copy link` directly accessible from the view. let's keep discussion of this temporary solution light. Signing off from Product and Design. Thanks so much @michael-s-molina !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776387670
+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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776374619
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776387670
+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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-779333060
In this last commit I did the following changes in addition to original changes:
- Changed icons to use Icon component except for JSON and CSV because we don’t have equivalent icons yet. Maybe with Diego's PR (https://github.com/apache/superset/pull/12229) we can replace these in the future.
- When clicked copy and email buttons were generating a different short URL even when the main URL had no change. I fixed this to avoid unnecessary POST requests.
- Added tooltip for all action buttons in Explore
- Removed outline border when active
<img width="294" alt="Screen Shot 2021-02-12 at 9 14 22 AM" src="https://user-images.githubusercontent.com/70410625/107971147-db4a8580-6f90-11eb-9d7a-19cdc6bd4c0e.png">
- Changed dashboard copy/email from modal to menu item
<img width="1767" alt="Screen Shot 2021-02-15 at 8 52 26 AM" src="https://user-images.githubusercontent.com/70410625/107971839-befb1880-6f91-11eb-9963-cf3e3c6f7758.png">
- Changed chart copy/email from modal to menu item
<img width="573" alt="Screen Shot 2021-02-15 at 8 52 10 AM" src="https://user-images.githubusercontent.com/70410625/107971589-6f1c5180-6f91-11eb-8ed5-dc37d86fc666.png">
I also created a React hook called `useUrlShortener` for the cases where we need a short version of an URL.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r573347583
##########
File path: superset-frontend/src/components/Button/index.tsx
##########
@@ -144,6 +144,17 @@ export default function Button(props: ButtonProps) {
colorHover = primary.base;
}
+ const element = children as ReactElement;
+
+ let renderedChildren = [];
+ if (element && element.type === React.Fragment) {
+ renderedChildren = Children.toArray(element.props.children);
+ } else {
+ renderedChildren = Children.toArray(children);
+ }
+
Review comment:
```suggestion
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zuzana-vej commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
zuzana-vej commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776386185
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-779498359
This is so cool. Thanks for all the updates and the attention to details! @michael-s-molina
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-777080877
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 ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776199390
# [Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=h1) Report
> Merging [#13037](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=desc) (0f40a2b) into [master](https://codecov.io/gh/apache/superset/commit/9cd01656dbe519f8735746b5f5fc08a53aa409ca?el=desc) (9cd0165) will **increase** coverage by `8.54%`.
> The diff coverage is `78.94%`.
[](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13037 +/- ##
==========================================
+ Coverage 53.38% 61.93% +8.54%
==========================================
Files 489 546 +57
Lines 17315 20161 +2846
Branches 4482 5275 +793
==========================================
+ Hits 9244 12486 +3242
+ Misses 8071 7462 -609
- Partials 0 213 +213
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `61.93% <78.94%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
| [...rset-frontend/src/components/URLShortLinkModal.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rTW9kYWwudHN4) | `74.07% <ø> (-3.71%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/welcome/ChartTable.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9DaGFydFRhYmxlLnRzeA==) | `76.08% <ø> (+72.86%)` | :arrow_up: |
| [...frontend/src/views/CRUD/welcome/DashboardTable.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9EYXNoYm9hcmRUYWJsZS50c3g=) | `62.50% <ø> (+60.50%)` | :arrow_up: |
| [...t-frontend/src/views/CRUD/welcome/SavedQueries.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9TYXZlZFF1ZXJpZXMudHN4) | `63.15% <ø> (+55.36%)` | :arrow_up: |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `70.45% <60.00%> (+13.31%)` | :arrow_up: |
| [superset-frontend/src/components/Button/index.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uL2luZGV4LnRzeA==) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/utils/copy.ts](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2NvcHkudHM=) | `19.23% <100.00%> (+15.06%)` | :arrow_up: |
| [superset-frontend/src/views/App.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0FwcC50c3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [462 more](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=footer). Last update [9cd0165...0f40a2b](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776338557
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-779518258
@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. ♥️
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776957184
> @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:
<img width="121" alt="Screen Shot 2021-02-10 at 4 01 52 PM" src="https://user-images.githubusercontent.com/70410625/107558340-6a365700-6bb9-11eb-909a-7f8a8cce1735.png">
I think the user will quickly assimilate, and also we have the tooltip to help. It could would look like this:
<img width="280" alt="Screen Shot 2021-02-10 at 4 24 17 PM" src="https://user-images.githubusercontent.com/70410625/107560708-766fe380-6bbc-11eb-9afe-a0c670c4caf2.png">
or
<img width="257" alt="Screen Shot 2021-02-10 at 4 21 57 PM" src="https://user-images.githubusercontent.com/70410625/107560496-314bb180-6bbc-11eb-896d-1a3e95e3868c.png">
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776374619
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 on the Copy icon. Putting the copy link in a dropdown menu might make the action flow smoother.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776199390
# [Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=h1) Report
> Merging [#13037](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=desc) (0f40a2b) into [master](https://codecov.io/gh/apache/superset/commit/9cd01656dbe519f8735746b5f5fc08a53aa409ca?el=desc) (9cd0165) will **increase** coverage by `19.74%`.
> The diff coverage is `78.94%`.
[](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=tree)
```diff
@@ 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
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `53.58% <77.77%> (+0.19%)` | :arrow_up: |
| javascript | `61.93% <78.94%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
| [...rset-frontend/src/components/URLShortLinkModal.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rTW9kYWwudHN4) | `81.48% <ø> (+3.70%)` | :arrow_up: |
| [...set-frontend/src/views/CRUD/welcome/ChartTable.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9DaGFydFRhYmxlLnRzeA==) | `76.08% <ø> (+72.86%)` | :arrow_up: |
| [...frontend/src/views/CRUD/welcome/DashboardTable.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9EYXNoYm9hcmRUYWJsZS50c3g=) | `62.50% <ø> (+60.50%)` | :arrow_up: |
| [...t-frontend/src/views/CRUD/welcome/SavedQueries.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9TYXZlZFF1ZXJpZXMudHN4) | `62.50% <ø> (+54.70%)` | :arrow_up: |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `77.27% <60.00%> (+20.12%)` | :arrow_up: |
| [superset-frontend/src/components/Button/index.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQnV0dG9uL2luZGV4LnRzeA==) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/utils/copy.ts](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3V0aWxzL2NvcHkudHM=) | `92.30% <100.00%> (+88.14%)` | :arrow_up: |
| [...tend/src/filters/components/Select/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvY29udHJvbFBhbmVsLnRz) | `18.18% <0.00%> (-81.82%)` | :arrow_down: |
| [superset-frontend/src/views/index.tsx](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2luZGV4LnRzeA==) | `25.00% <0.00%> (-75.00%)` | :arrow_down: |
| ... and [412 more](https://codecov.io/gh/apache/superset/pull/13037/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=footer). Last update [9cd0165...0f40a2b](https://codecov.io/gh/apache/superset/pull/13037?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud merged pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #13037:
URL: https://github.com/apache/superset/pull/13037
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-779502618
@ktmud you wanna approve the code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zuzana-vej commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
zuzana-vej commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-777116705
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776387670
+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 something that's closer to the other design.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776957184
> @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:
<img width="121" alt="Screen Shot 2021-02-10 at 4 01 52 PM" src="https://user-images.githubusercontent.com/70410625/107558340-6a365700-6bb9-11eb-909a-7f8a8cce1735.png">
I think the user will quickly assimilate, and also we have the tooltip to help. It would look like this:
<img width="317" alt="Screen Shot 2021-02-10 at 4 34 38 PM" src="https://user-images.githubusercontent.com/70410625/107561992-fe0a2200-6bbd-11eb-96f5-98beae4c2534.png">
or
<img width="287" alt="Screen Shot 2021-02-10 at 4 35 14 PM" src="https://user-images.githubusercontent.com/70410625/107562018-082c2080-6bbe-11eb-95e4-233a5aa57ff2.png">
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577631576
##########
File path: superset-frontend/src/explore/components/ExploreActionButtons.jsx
##########
@@ -115,6 +163,19 @@ export default function ExploreActionButtons({
/>
</div>
);
-}
+};
+
+ExploreActionButtons.propTypes = {
+ actions: PropTypes.object.isRequired,
+ canDownload: PropTypes.oneOfType([PropTypes.string, PropTypes.bool])
+ .isRequired,
+ chartStatus: PropTypes.string,
+ chartHeight: PropTypes.string.isRequired,
+ latestQueryFormData: PropTypes.object,
+ queriesResponse: PropTypes.arrayOf(PropTypes.object),
+ slice: PropTypes.object,
+ addDangerToast: PropTypes.func.isRequired,
+ addSuccessToast: PropTypes.func.isRequired,
+};
Review comment:
Updated to typescript!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r576477468
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
+
+ async function onCopyLink() {
+ try {
+ await copyTextToClipboard(getShortUrl());
+ addSuccessToast(t('Copied to clipboard!'));
+ } catch (error) {
+ addDangerToast(
+ t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'),
Review comment:
I don't think there is a place to `Ctrl + C` from anymore after the updated UI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776932740
@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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r576476076
##########
File path: superset-frontend/src/explore/components/ExploreActionButtons.jsx
##########
@@ -115,6 +163,19 @@ export default function ExploreActionButtons({
/>
</div>
);
-}
+};
+
+ExploreActionButtons.propTypes = {
+ actions: PropTypes.object.isRequired,
+ canDownload: PropTypes.oneOfType([PropTypes.string, PropTypes.bool])
+ .isRequired,
+ chartStatus: PropTypes.string,
+ chartHeight: PropTypes.string.isRequired,
+ latestQueryFormData: PropTypes.object,
+ queriesResponse: PropTypes.arrayOf(PropTypes.object),
+ slice: PropTypes.object,
+ addDangerToast: PropTypes.func.isRequired,
+ addSuccessToast: PropTypes.func.isRequired,
+};
Review comment:
Next time, if we are to update `propTypes`, might as well just convert the whole component into `tsx` first.
You could also just convert it to a functional component and use the `useToasts` hook.
##########
File path: superset-frontend/spec/javascripts/dashboard/components/HeaderActionsDropdown_spec.jsx
##########
@@ -79,19 +79,19 @@ describe('HeaderActionsDropdown', () => {
expect(menu.find(SaveModal)).not.toExist();
});
- it('should render five Menu items', () => {
+ it('should render four Menu items', () => {
Review 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.
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
+
+ async function onCopyLink() {
+ try {
+ await copyTextToClipboard(getShortUrl());
+ addSuccessToast(t('Copied to clipboard!'));
+ } catch (error) {
+ addDangerToast(
+ t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'),
+ );
+ }
+ }
+
+ async function onShareByEmail() {
+ try {
+ const bodyWithLink = t('%s%s', emailBody, getShortUrl());
Review comment:
`t` is probably not needed if this is just simple string concatenation.
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
Review comment:
I think you can just let `useUrlShortener` return `shortUrl` itself and add a loading state if `shortUrl` is falsy.
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
+
+ async function onCopyLink() {
+ try {
+ await copyTextToClipboard(getShortUrl());
+ addSuccessToast(t('Copied to clipboard!'));
+ } catch (error) {
+ addDangerToast(
+ t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'),
Review comment:
I don't think there is a place to `Ctrl + C` from after the updated UI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577630904
##########
File path: superset-frontend/spec/javascripts/dashboard/components/HeaderActionsDropdown_spec.jsx
##########
@@ -79,19 +79,19 @@ describe('HeaderActionsDropdown', () => {
expect(menu.find(SaveModal)).not.toExist();
});
- it('should render five Menu items', () => {
+ it('should render four Menu items', () => {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577979866
##########
File path: superset-frontend/src/explore/components/ExploreActionButtons.tsx
##########
@@ -0,0 +1,204 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState } from 'react';
+import cx from 'classnames';
+import { t } from '@superset-ui/core';
+import Icon from 'src/components/Icon';
+import { Tooltip } from 'src/common/components/Tooltip';
+import copyTextToClipboard from 'src/utils/copy';
+import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import EmbedCodeButton from './EmbedCodeButton';
+import ConnectedDisplayQueryButton from './DisplayQueryButton';
+import { exportChart, getExploreLongUrl } from '../exploreUtils';
+
+type ActionButtonProps = {
+ icon: React.ReactElement;
+ text?: string;
+ tooltip: string;
+ className?: string;
+ onClick: React.MouseEventHandler<HTMLElement>;
+ onTooltipVisibilityChange?: (visible: boolean) => void;
+ 'data-test'?: string;
+};
+
+type ExploreActionButtonsProps = {
+ actions: { redirectSQLLab: Function; openPropertiesModal: Function };
+ canDownload: boolean;
+ chartHeight: number;
+ chartStatus: string;
+ latestQueryFormData: {};
+ queriesResponse: {};
+ slice: { slice_name: string };
+ addDangerToast: Function;
+};
+
+const ActionButton = (props: ActionButtonProps) => {
+ const {
+ icon,
+ text,
+ tooltip,
+ className,
+ onTooltipVisibilityChange,
+ ...rest
+ } = props;
+ return (
+ <Tooltip
+ id={`${icon}-tooltip`}
+ placement="top"
+ title={tooltip}
+ trigger={['hover']}
+ onVisibleChange={onTooltipVisibilityChange}
+ >
+ <div
+ role="button"
+ tabIndex={0}
+ css={{ '&:focus, &:focus:active': { outline: 0 } }}
+ className={className || 'btn btn-default btn-sm'}
+ style={{ height: 30 }}
+ {...rest}
+ >
+ {icon}
+ {text && <span style={{ marginLeft: 5 }}>{text}</span>}
+ </div>
+ </Tooltip>
+ );
+};
+
+const ExploreActionButtons = (props: ExploreActionButtonsProps) => {
+ const {
+ actions,
+ canDownload,
+ chartHeight,
+ chartStatus,
+ latestQueryFormData,
+ queriesResponse,
+ slice,
+ addDangerToast,
+ } = props;
+
+ const copyTooltipText = t('Copy chart URL to clipboard');
+ const [copyTooltip, setCopyTooltip] = useState(copyTooltipText);
+ const longUrl = getExploreLongUrl(latestQueryFormData);
+ const getShortUrl = useUrlShortener(longUrl);
+
+ const doCopyLink = async () => {
+ 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.'));
+ }
Review comment:
```suggestion
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577635477
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
+
+ async function onCopyLink() {
+ try {
+ await copyTextToClipboard(getShortUrl());
+ addSuccessToast(t('Copied to clipboard!'));
+ } catch (error) {
+ addDangerToast(
+ t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'),
Review comment:
Great catch! Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776199390
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r576476076
##########
File path: superset-frontend/src/explore/components/ExploreActionButtons.jsx
##########
@@ -115,6 +163,19 @@ export default function ExploreActionButtons({
/>
</div>
);
-}
+};
+
+ExploreActionButtons.propTypes = {
+ actions: PropTypes.object.isRequired,
+ canDownload: PropTypes.oneOfType([PropTypes.string, PropTypes.bool])
+ .isRequired,
+ chartStatus: PropTypes.string,
+ chartHeight: PropTypes.string.isRequired,
+ latestQueryFormData: PropTypes.object,
+ queriesResponse: PropTypes.arrayOf(PropTypes.object),
+ slice: PropTypes.object,
+ addDangerToast: PropTypes.func.isRequired,
+ addSuccessToast: PropTypes.func.isRequired,
+};
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-780579786
> Code LGTM with a couple of small comments.
>
> A small UX suggestion: instead of using toast, you may also just updated the tooltip when copied was successful (you can even show a "Loading..." while fetching the shortened URL in the tooltip, too)
>
> <img alt="copied-to-clipboard" width="324" src="https://user-images.githubusercontent.com/335541/108005424-8c5f1900-6fad-11eb-9cb6-4700e3fb20b3.png">
@ktmud Accepted you suggestion. Now we update the tooltip.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577635102
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776387670
+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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #13037:
URL: https://github.com/apache/superset/pull/13037#discussion_r577637055
##########
File path: superset-frontend/src/dashboard/components/menu/ShareMenuItems.tsx
##########
@@ -0,0 +1,85 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useUrlShortener } from 'src/common/hooks/useUrlShortener';
+import copyTextToClipboard from 'src/utils/copy';
+import { t } from '@superset-ui/core';
+import { Menu } from 'src/common/components';
+
+interface ShareMenuItemProps {
+ url: string;
+ copyMenuItemTitle: string;
+ emailMenuItemTitle: string;
+ emailSubject: string;
+ emailBody: string;
+ addDangerToast: Function;
+ addSuccessToast: Function;
+}
+
+const ShareMenuItems = (props: ShareMenuItemProps) => {
+ const {
+ url,
+ copyMenuItemTitle,
+ emailMenuItemTitle,
+ emailSubject,
+ emailBody,
+ addDangerToast,
+ addSuccessToast,
+ ...rest
+ } = props;
+
+ const getShortUrl = useUrlShortener(url);
+
+ async function onCopyLink() {
+ try {
+ await copyTextToClipboard(getShortUrl());
+ addSuccessToast(t('Copied to clipboard!'));
+ } catch (error) {
+ addDangerToast(
+ t('Sorry, your browser does not support copying. Use Ctrl / Cmd + C!'),
+ );
+ }
+ }
+
+ async function onShareByEmail() {
+ try {
+ const bodyWithLink = t('%s%s', emailBody, getShortUrl());
Review comment:
I checked that the parent component is responsible for the translation of email body. No need here. Removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-779515036
> @ktmud you wanna approve the code?
Was just reviewing and testing this locally. 😃
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-777840169
We have plan of moving those buttons and share under menu, leaving only `copy link` directly accessible from the view. let's keep discussion of this temporary solution light. Signing off from Product and Design. Thanks so much @michael-s-molina !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina edited a comment on pull request #13037: feat: add autocopy functionality to CopyToClipboard (#10328)
Posted by GitBox <gi...@apache.org>.
michael-s-molina edited a comment on pull request #13037:
URL: https://github.com/apache/superset/pull/13037#issuecomment-776957184
> @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:
<img width="121" alt="Screen Shot 2021-02-10 at 4 01 52 PM" src="https://user-images.githubusercontent.com/70410625/107558340-6a365700-6bb9-11eb-909a-7f8a8cce1735.png">
I think the user will quickly assimilate, and also we have the tooltip to help. It would look like this:
<img width="280" alt="Screen Shot 2021-02-10 at 4 24 17 PM" src="https://user-images.githubusercontent.com/70410625/107560708-766fe380-6bbc-11eb-9afe-a0c670c4caf2.png">
or
<img width="257" alt="Screen Shot 2021-02-10 at 4 21 57 PM" src="https://user-images.githubusercontent.com/70410625/107560496-314bb180-6bbc-11eb-896d-1a3e95e3868c.png">
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org