Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Copy icons to clipboard #23850

Merged
merged 8 commits into from
Dec 6, 2020

Conversation

codewithguruji
Copy link
Contributor

@codewithguruji codewithguruji commented Dec 5, 2020

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 5, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 7edc47f

@mbrookes mbrookes added the docs Improvements or additions to the documentation label Dec 5, 2020
@mbrookes
Copy link
Member

mbrookes commented Dec 5, 2020

Thanks for working on it. I think we should:

  • Add a "copy" button. Copying when selecting or just clicking the text is unexpected. We don't know what might be in the user's clipboard that we just splatted.
  • Add a notification similar to that on the "copy the source" button in the demos, so that the user knows that the text has been successfully copied.
  • Remove the automatic text highlight, since this breaks expectations for a single click on some text.

@oliviertassinari oliviertassinari changed the title [Material Icons] Copy to clipboard [docs] Copy icons to clipboard Dec 5, 2020
@oliviertassinari oliviertassinari self-assigned this Dec 5, 2020
@oliviertassinari oliviertassinari removed their assignment Dec 5, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 5, 2020

I have tried to experiment with this but I couldn't come up with anything great without dedicated a disproportionate amount of time. I have found two things that suck. 1. the snackbar is a nightmare to use: #18098, 2. the dialog title bundle two DOM nodes #19696 which is surprising.

@oliviertassinari oliviertassinari self-assigned this Dec 5, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 5, 2020

Actually, I think that I have found a nice trick, using https://ionicons.com/, https://fontawesome.com/icons/archway?style=duotone and GitHub copy button as inspiration:

Capture d’écran 2020-12-05 à 13 51 15

@codewithguruji @mbrookes What do you think about the latest version? (It has cost me ~30 minutes, could it be a quick win?)

copy

https://deploy-preview-23850--material-ui.netlify.app/components/material-icons/#main-content

@oliviertassinari oliviertassinari removed their assignment Dec 5, 2020
@mbrookes
Copy link
Member

mbrookes commented Dec 5, 2020

Definitely an improvement - the only thing I might add is "click to copy" on the tooltip. It isn't unreasonable to assume that a click is with the intention to copy though, so probably good enough.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 5, 2020

the only thing I might add is "click to copy" on the tooltip.

FontAwesome does that, I agree, it might work even better. I won't spend time on it, but if somebody wants to push it further, I will be curious to see the result :)

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mbrookes
Copy link
Member

mbrookes commented Dec 5, 2020

I think that these 3 labels should be generic (not scoped under searchIcons)

I originally thought the same (in general), but since they will dedupe on CrowdIn, there's no overhead. Doing it this way reduces the chance of accidentally removing a translation in use on another page. It should also make it easier to spot dead translations.

(I also noticed that you had moved potentially shareable translations in the API page under a page specific key.)

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 5, 2020

I originally thought the same (in general), but since they will dedupe on CrowdIn, there's no overhead. Doing it this way reduces the chance of accidentally removing a translation in use on another page. It should also make it easier to spot dead translations.

I don't think that spotting dead translations should be a variable on the tradeoff as it can be automated. I don't think that accidentally removing a translation is a variable either because it's easy to grep in the codebase. I think that what's important is whether or not we want to anticipate the need to provide custom translation for each context. IMHO, the best tradeoff is to optimize for English and to a "best-effort" for the other locales (so we spend as little time as possible on them) => no duplication.

@oliviertassinari oliviertassinari merged commit ea8060d into mui:next Dec 6, 2020
@oliviertassinari
Copy link
Member

@codewithguruji Thanks for working on this :)

@codewithguruji
Copy link
Contributor Author

@codewithguruji Thanks for working on this :)

Thank you so much @oliviertassinari @mbrookes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants