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

Copy button - Details page #5832

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

trmendes
Copy link
Contributor

@trmendes trmendes commented Feb 5, 2024

What does this PR do?

It makes usage of the “Copy To Clipboard” component, so the users can copy the title of the details page to the clipboard.

Screenshot / video of UI

copy_button_details

What issues does this PR fix or reference?

Fixes #4241

How to test this PR?

  1. Open the detail page
  2. Click on the copy button
  3. Check if the title is present on your clipboard as expected

@trmendes trmendes requested review from benoitf and a team as code owners February 5, 2024 09:30
@trmendes trmendes requested review from jeffmaury and feloy and removed request for a team February 5, 2024 09:30
@trmendes
Copy link
Contributor Author

trmendes commented Feb 5, 2024

Not very happy with the layout of it. Any suggestions and hints for the CSS?

@benoitf
Copy link
Collaborator

benoitf commented Feb 5, 2024

I think it's around the vertical alignment (I have not looked at the html elements being currently used to give you the proper answer but hints would be around items-center, align-middle, etc

@trmendes trmendes force-pushed the 4241_copy_button_details_page branch from ccf48ae to de8e125 Compare February 6, 2024 07:55
let the one who is using the component to decide what is the text size to use
Signed-off-by: Thiago Mendes <[email protected]>
make usage of copy to clipboard component into detail pages
Signed-off-by: Thiago Mendes <[email protected]>
adapt current test to find heading that is not encapsulated into copy to clipboard component
Signed-off-by: Thiago Mendes <[email protected]>
@trmendes trmendes force-pushed the 4241_copy_button_details_page branch from de8e125 to 8bb042f Compare February 6, 2024 08:14
@trmendes
Copy link
Contributor Author

trmendes commented Feb 6, 2024

@dgolovin Thank you do much for your guidance with CSS :)

It looks much better now <3

copy1

copy2

@benoitf
Copy link
Collaborator

benoitf commented Feb 6, 2024

I find confusing the clipboard button being in the middle of the line as I'm not sure what it could copy

image

I don't know if we need to move the sha to a newline or think in a different way or just copy the sha and not the image name (so button is at the right)

@trmendes
Copy link
Contributor Author

trmendes commented Feb 6, 2024

@benoitf I completely agree, and I experienced the same feeling of confusion when opening the panel to take the screenshot.

I might be a bit busy with an interview this week, but I will come back to it, do some experiments and post screenshots here as soon as possible.

Thanks for your comment :)

@benoitf
Copy link
Collaborator

benoitf commented Feb 6, 2024

@trmendes good stuff anyway

maybe to be less confusing it's to have the icon appearing only on hover and a different background including both the thing to copy and the copy button so we see that it's the 'same unit'

I'll let others comment here

@trmendes
Copy link
Contributor Author

trmendes commented Feb 6, 2024

@benoitf I took a quick look at docker desktop to see how they handle that sort of information, and they actually have a copy button for the hash and not for the object name. They also print the tag along with the name.

What is the reason(s) we choose copying the name over hash?

@benoitf
Copy link
Collaborator

benoitf commented Feb 6, 2024

cc @mairin

@axel7083
Copy link
Contributor

axel7083 commented Feb 6, 2024

cc @mairin

I had something similar open long time ago see @mairin comment on the subject #4242 (comment)

@mairin
Copy link
Member

mairin commented Feb 6, 2024

@axel7083 Yep exactly what I was gonna suggest :) Here's a prototype, it's a little hairy but you get the idea:

https://design.penpot.app/#/view/43442e9d-d45f-8169-8003-daff3cb3e0b8?page-id=43442e9d-d45f-8169-8003-daff3cb3e0b9&section=interactions&index=0&share-id=93d0ad32-dfe5-8194-8003-db0444b2c3bf

Screencast.from.2024-02-06.10-34-52.webm

@feloy
Copy link
Contributor

feloy commented Feb 6, 2024

I find confusing the clipboard button being in the middle of the line as I'm not sure what it could copy

image

I don't know if we need to move the sha to a newline or think in a different way or just copy the sha and not the image name (so button is at the right)

I would move the icon closer to the text it copies. Here it is exactly in the middle, the reason why IMO it is confusing.
302550209-975bb917-3a7f-4e28-90e9-cbf496c128a8

@deboer-tim
Copy link
Collaborator

+1 to design from @mairin. That's a much cleaner look, which means we could use it in far more places (e.g. we don't need to decide between name or hash, both could allow copying without any UI clutter or confusion).

CopyToClipboard is already used in Preferences > Resources page (#5687), I think the question is whether we use it here basically as-is or refactor it to the design first. Since there are multiple comments on placement I'd prefer the latter, also so that we wouldn't add buttons and then 'remove' them later.

@feloy
Copy link
Contributor

feloy commented Feb 6, 2024

+1 to design from @mairin. That's a much cleaner look, which means we could use it in far more places (e.g. we don't need to decide between name or hash, both could allow copying without any UI clutter or confusion).

CopyToClipboard is already used in Preferences > Resources page (#5687), I think the question is whether we use it here basically as-is or refactor it to the design first. Since there are multiple comments on placement I'd prefer the latter, also so that we wouldn't add buttons and then 'remove' them later.

My only concern about these popup on which we need to click (not specifically here, but in different apps/websites), is related to accessibility and that you have to be quite precise with your mouse and don't move too much, or the popup disappears before you can click on it. And is it usable with keyboard only?

@deboer-tim
Copy link
Collaborator

My only concern about these popup on which we need to click (not specifically here, but in different apps/websites), is related to accessibility and that you have to be quite precise with your mouse and don't move too much, or the popup disappears before you can click on it.

Agreed, we need to make sure it is not finicky and mouse-over area is generous. Could be other things we could do too, e.g. maybe clicking on the text keeps the popup open?

And is it usable with keyboard only?

FWIW, I just checked what to me is the 'Mac equivalent': copying a password from the password manager. When you're using the keyboard you never see the popup - when you tab in it auto-selects all the text and a normal Ctrl-C works fine.

@mairin
Copy link
Member

mairin commented Feb 6, 2024

@deboer-tim Ah that is what I would suggest for keyboard... just enable ctrl+c on copyable fields on focus.

@benoitf
Copy link
Collaborator

benoitf commented Mar 12, 2024

converting do draft

@benoitf benoitf marked this pull request as draft March 12, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add copy button next to title in details page
7 participants