-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use copy button component for the plots ribbon experiments #1812
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works for me on a manual test!
I do think the buttons appearing to the side and reflowing the ribbon is a bit jarring, maybe a better looking solution would be to have the ribbon item expand downward on hover to reveal the control buttons? Either way, I think this is a fine inclusion as is.
Code Climate has analyzed commit 2056da7 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.9% (0.1% change). View more on Code Climate. |
Thanks, @sroy3 . Agreed with Roger that we should show icon to the right in this case (truncate name if needed). (or some other way to handle this) |
I'll follow up on this |
Looks good @sroy3! I would just mack sure the single block won't change its and parent container widths, so the user won't experience unnecessary movement, but just icon will be shown. WDYT? |
Already done in another PR. Thanks. |
Screen.Recording.2022-06-01.at.10.52.07.AM.mov