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

chore: add copy to clipboard button to resources page #5687

Merged
merged 6 commits into from Feb 2, 2024

Conversation

trmendes
Copy link
Contributor

What does this PR do?

This PR adds a new button to the resources page. The added button provides the user with a way to copy the provider endpoint to the clipboard.

Screenshot / video of UI

Screenshot 2024-01-25 at 11 17 52

What issues does this PR fix or reference?

Fixes #5608

How to test this PR?

  1. Run Podman Desktop from this branch
  2. Go to Settings > Resources
  3. Check if the copy icon stands next to the resource endpoint
  4. Click on it
  5. Verify if the endpoint text is on your clipboard

@benoitf
Copy link
Collaborator

benoitf commented Jan 25, 2024

Hello @trmendes I will comment here what you asked #5608 (comment)

as here it's where it's implemented so it feels to be a better place

instead of trying to add some svelte intructions to grab OS, etc

first, do a new Svelte file to introduce the clipboard option for the resource

for example let say PreferencesResourcesRenderingCopyButton (or another name)

then, it will avoid to have copy functions, etc whatever in the PreferencesResourcesRendering.svelte file

then, in this file, you can use some typescript stuff during the onMount( callback
this is where you can fetch the platform of the user

and you'll add an exposed property: the path that you wanted to copy

and what you'll do is:

fetch OS, convert path to the new value

and then in the html section of the svelte file, you'll just check that if you have the encoded path value, you display the copy widget

@trmendes
Copy link
Contributor Author

trmendes commented Jan 25, 2024

Thank you for your feedback and description @benoitf

I'm completely new to svelte and vitests, and I'm trying to figure it out things on the go so, I really appreciate your help.

I hope to be able to bring more to the project as soon as I get more familiarized with the code and the frameworks.

If I've got it right, the idea is to create a new component and give this new component the path to be transformed. Later, when the button is clicked, that transformed string will be the one to save to the clipboard.

Would you mind taking a look at the code I submit?

Add a button to copy the resource socket address to the clipboard.

Signed-off-by: Thiago Ribeiro Mendes <tzig@tutanota.de>
Signed-off-by: Thiago Mendes <tzig@tutanota.de>
A test to verify the copy to clipboard button added to the resources page

Signed-off-by: Thiago Ribeiro Mendes <tzig@tutanota.de>
Signed-off-by: Thiago Mendes <tzig@tutanota.de>
@trmendes trmendes marked this pull request as ready for review January 25, 2024 23:24
@trmendes trmendes requested a review from a team as a code owner January 25, 2024 23:24
@trmendes trmendes requested review from jeffmaury and cdrage and removed request for a team January 25, 2024 23:24
@deboer-tim
Copy link
Collaborator

I'm pretty sure you've both seen the discussion in PR #4242, but to me the same discussion applies here - having a copy button is distracting and in this case takes up space that could show more of the endpoint. Personally I was hoping we could figure out something similar to Tooltip that was generic and easy to use with text anywhere, where hovering or clicking would show you a copy button.

I don't want to stifle innovation or contributions in the meantime though... maybe we just need to be careful about doing one-off solutions in too many places.

@benoitf
Copy link
Collaborator

benoitf commented Jan 26, 2024

@trmendes ok nice enhancement

so now, you see that you've a PreferencesResourcesRenderingCopyButton.svelte component that extracts the URL and then make usage of Tooltip component.

 <Tooltip tip="Copy to Clipboard" bottom>
    <button
      title="Copy To Clipboard"
      class="ml-5"
      aria-label="Copy To Clipboard"
      on:click="{() => copyResourceSocketAddrToClipboard()}">
      <Fa icon="{faPaste}" />
    </button>
  </Tooltip>

and then we need to do :

<div class="float-right">
   <PreferencesResourcesRenderingCopyButton path="{container.endpoint.socketPath}" />
</div>
<div
   class="mt-1 my-auto text-xs truncate"
   class:text-gray-900="{container.status !== 'started'}"
   aria-label="{container.name} endpoint"
   title="{container.endpoint.socketPath}">
   {container.endpoint.socketPath}
</div>

One thing that we see is that container.endpoint.socketPath is used 3 times in this former section

so probably the copy/paste option could be more generic like as a client you may want to do

<PreferencesResourcesRenderingSocketPath title="{container.endpoint.socketPath}" class:text-gray-900="{container.status !== 'started'}">

this component will compute the URL and then call the display the detail thing with the clipboard option

{#if url}
<CopyToClipboard title={title} clipboardData="{url}" />
{/if}

CopyToClipboard containing the display of the field and the button to copy the link

with that it means that if I want later one display another field with a copy/paste button I only need to call

<CopyToClipboard title={title} clipboardData="{url}" />

And then, we could iterate to have display button in case of hovering, always one, being a user preference etc but all fields will behave in the same manner

Do you think you can work on this ?

@trmendes
Copy link
Contributor Author

trmendes commented Jan 26, 2024

I have the changes done by now, but I'm not sure how to test PreferencesResourcesRenderingSocketPath.

I thought about being sure the title is there and that clipboardData is the expected URL but missing a way to do it. Any hints on what methods to use?

Reading the debug messages, I can't see it on the HTML. Maybe I'm missing some concepts.

Meanwhile, I'm going to read on how to add classes to components :D

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍
I think it'll allow to then decide if to move forward using copy on hover, or whatever option but at least the copying feature will be there

Add a generic copy button to copy the displayed text to the clipboard.
Signed-off-by: Thiago Mendes <tzig@tutanota.de>
Test our Generic copy button component
Signed-off-by: Thiago Mendes <tzig@tutanota.de>
Copy link
Contributor Author

@trmendes trmendes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm propagating the class now but I still wonder about the tests for the PreferencesResourcesRenderingCopyButton.

@benoitf
Copy link
Collaborator

benoitf commented Jan 26, 2024

I still wonder about the tests for the PreferencesResourcesRenderingCopyButton..

the role of this component is to transform a link and add npipe or unix:

so tests should be about:

  • check that title contains the path
  • if you click on copy to clipboard the clipboard contains the value with npipe or unix:

@trmendes
Copy link
Contributor Author

The copy to clipboard test is done on the packages/renderer/src/lib/ui/CopyToClipboard.spec.ts

For the other component packages/renderer/src/lib/preferences/PreferencesResourcesRenderingCopyButton.spec.ts:

I wanted to test if the URL conversion is working as expected, but since I'm not finding ways to get access to it, I'm not sure how to do it.

@benoitf
Copy link
Collaborator

benoitf commented Jan 26, 2024

wanted to test if the URL conversion is working as expected, but since I'm not finding ways to get access to it, I'm not sure how to do it.

clicking on the clipboard button and validating would work

if you want to test only the url
I guess result.component.$$.ctx[0] should contain the value to check (or another index)

@benoitf
Copy link
Collaborator

benoitf commented Jan 28, 2024

hello @trmendes it looks like unit tests are failing on the CI

src/lib/preferences/PreferencesResourcesRendering.spec.ts > Expect type to be reported for Podman engines
→ Unable to find a label with the text of: machine endpoint

https://github.com/containers/podman-desktop/actions/runs/7670613844/job/20927773826?pr=5687

@trmendes trmendes force-pushed the 5608_endpoint_copy_button branch 2 times, most recently from 44e771e to 2738547 Compare January 29, 2024 09:36
@trmendes
Copy link
Contributor Author

@benoitf Thanks for the heads-up.

I believe those lines that are failing because I moved what they are testing to the generic component.

My approach is to remove those 2 lines of each of two failing tests because they are already being tested by the other component.

Does it sound ok to you?

@trmendes
Copy link
Contributor Author

I also tried to debug the DOM and see what is there after rendering, but my knowledge on svelte and vistest are still small.

After looking through the documentation and I end up surrounding the generic component with a div so, I could get it and print what was the HTML nested to the div.

What I see after printing it was an empty div. So, I wonder if the components we create are rendered during the tests. Not sure if I'm being clear. Let me know if you have any questions.

I plan to what a course on svelte soon so I will have more knowledge to work on some other tasks.

@benoitf
Copy link
Collaborator

benoitf commented Jan 29, 2024

I believe those lines that are failing because I moved what they are testing to the generic component.
My approach is to remove those 2 lines of each of two failing tests because they are already being tested by the other component.

Does it sound ok to you?

I would keep the lines

But maybe it's related to the delayed onMount that is not rendering the text quickly but the test is executed before it's finished

so probably need to wait the renderer part

@benoitf
Copy link
Collaborator

benoitf commented Jan 29, 2024

I also tried to debug the DOM and see what is there after rendering, but my knowledge on svelte and vistest are still small.

After looking through the documentation and I end up surrounding the generic component with a div so, I could get it and print what was the HTML nested to the div.

What I see after printing it was an empty div. So, I wonder if the components we create are rendered during the tests. Not sure if I'm being clear. Let me know if you have any questions.

AFAIK it's because you're hitting the {if #url} condition not matching as test is executed before it had time to do all the queries, etc.

Move from having a specific component to a more generic one

Signed-off-by: Thiago Mendes <tzig@tutanota.de>
@trmendes
Copy link
Contributor Author

trmendes commented Jan 29, 2024

I also tried to debug the DOM and see what is there after rendering, but my knowledge on svelte and vistest are still small.

After looking through the documentation and I end up surrounding the generic component with a div so, I could get it and print what was the HTML nested to the div.

What I see after printing it was an empty div. So, I wonder if the components we create are rendered during the tests. Not sure if I'm being clear. Let me know if you have any questions.

AFAIK it's because you're hitting the {if #url} condition not matching as test is executed before it had time to do all the queries, etc.

Super! That was it! I'm now waiting for the component to be rendered, so I can run the query and the expectation check! Thanks for all the help and guidance you are providing me.

Waiting for the test results :)

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @trmendes

@trmendes
Copy link
Contributor Author

Thanks, @benoitf, for all your help and support.

@benoitf
Copy link
Collaborator

benoitf commented Feb 2, 2024

forgot to merge the PR 🤦

@benoitf benoitf merged commit c22ff63 into containers:main Feb 2, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.8.0 milestone Feb 2, 2024
@trmendes trmendes deleted the 5608_endpoint_copy_button branch February 2, 2024 12:35
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.

Hide the endpoint path from the summary, only show it in the detail
4 participants