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

Add a copy icon to entries in KeyValuesTable (#204) #292

Merged

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Dec 13, 2018

Which problem is this PR solving?

  • Tags are human-readable ( 🎉), but harder to paste into other tools.

Short description of the changes

  • Add icon to rows in KeyValueTable to copy that row's data.

204 copy tags with tooltip
(again, my converted gifs have a weird background color issue, looks fine in the browser and .webm recording)

Copied JSON:

{
  "key": "sql.query",
  "type": "string",
  "value": "SELECT * FROM customer WHERE customer_id=731"
}

I went with the copy icon over the word copy since I believe it is fairly ubiquitous, but open to changing.

@yurishkuro
Copy link
Member

I don't love that there is no "copy confirmation" so I welcome advice on how to indicate that to the user.

the copy icon can be animated to transform to a green checkbox.

Q: in what format is the tag being copied? I.e. how does the above example look after pasting it as text?

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

I would suggest using the tooltip on the icon to state / indicate:

  • "Copy JSON" on mouseover
  • "Copied to clipboard" on click
    • Set a mouseLeaveDelay to something like 500ms (or so) to let the user more easily read the message

What do you think?

@ghost ghost assigned everett980 Dec 19, 2018
@ghost ghost added the review label Dec 19, 2018
@everett980
Copy link
Collaborator Author

Q: in what format is the tag being copied? I.e. how does the above example look after pasting it as text?

@yurishkuro the JSON.stringify(row, null, 2) looks like the following:

{
  "key": "hostname",
  "type": "string",
  "value": "6376c38cb700"
}

The null, 2 arguments add new lines with two spaces for indentation. It helps considerably with legibility and just about any tool that accepts JSON input can handle the white-space characters.

@everett980
Copy link
Collaborator Author

the copy icon can be animated to transform to a green checkbox.

I would suggest using the tooltip on the icon to state / indicate:

  • "Copy JSON" on mouseover
  • "Copied to clipboard" on click
    • Set a mouseLeaveDelay to something like 500ms (or so) to let the user more easily read the message
      What do you think?

@yurishkuro @tiffon I went with the tooltip over the icon change as the tooltip will help with clarity for both before and after the copy event.
antd's tooltip struggles with a left-positioned tooltip's title increasing in length, so I used the title "Copy JSON" before the row has been copied, and "Copied" for after copy. I would hope that a user wouldn't be reliant on the "to clipboard" detail to know where copied data goes. I will update the gif in the description to include the tooltips.

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Changes look great. A few small comments. There is an issue with the yarn.lock file.

@codecov
Copy link

codecov bot commented Jan 3, 2019

Codecov Report

Merging #292 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   82.23%   82.23%   -0.01%     
==========================================
  Files         141      141              
  Lines        3101     3112      +11     
  Branches      645      647       +2     
==========================================
+ Hits         2550     2559       +9     
- Misses        437      439       +2     
  Partials      114      114
Impacted Files Coverage Δ
...nts/TracePage/TraceTimelineViewer/SpanDetailRow.js 100% <ø> (ø) ⬆️
...omponents/TraceDiff/TraceDiffHeader/TraceHeader.js 14.28% <ø> (ø) ⬆️
packages/jaeger-ui/src/components/App/NotFound.js 0% <ø> (ø) ⬆️
...e/TraceTimelineViewer/SpanDetail/KeyValuesTable.js 90.9% <100%> (+4.54%) ⬆️
...neViewer/TimelineHeaderRow/TimelineViewingLayer.js 87.03% <0%> (-3.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a0cf66...e1ffeda. Read the comment docs.

@ghost ghost assigned tiffon Jan 4, 2019
@tiffon tiffon merged commit d1d258a into jaegertracing:master Jan 4, 2019
@ghost ghost removed the review label Jan 4, 2019
everett980 added a commit to everett980/jaeger-ui that referenced this pull request Jan 16, 2019
…gertracing#292)

* Add a copy icon to entries in KeyValuesTable (jaegertracing#204)

Signed-off-by: Everett Ross <[email protected]>

* Add a tooltip to copy icon in KeyValuesTable

Signed-off-by: Everett Ross <[email protected]>

* Fix copied test name, add test for KeyValuesTable state change on tooltip hide

Signed-off-by: Everett Ross <[email protected]>

* Add eslint rule to prevent unnecessary braces in jsx

Signed-off-by: Everett Ross <[email protected]>

* Add classname to tr to remove element selector, fix yarn.lock

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…gertracing#292)

* Add a copy icon to entries in KeyValuesTable (jaegertracing#204)

Signed-off-by: Everett Ross <[email protected]>

* Add a tooltip to copy icon in KeyValuesTable

Signed-off-by: Everett Ross <[email protected]>

* Fix copied test name, add test for KeyValuesTable state change on tooltip hide

Signed-off-by: Everett Ross <[email protected]>

* Add eslint rule to prevent unnecessary braces in jsx

Signed-off-by: Everett Ross <[email protected]>

* Add classname to tr to remove element selector, fix yarn.lock

Signed-off-by: Everett Ross <[email protected]>

Signed-off-by: vvvprabhakar <[email protected]>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…gertracing#292)

* Add a copy icon to entries in KeyValuesTable (jaegertracing#204)

Signed-off-by: Everett Ross <[email protected]>

* Add a tooltip to copy icon in KeyValuesTable

Signed-off-by: Everett Ross <[email protected]>

* Fix copied test name, add test for KeyValuesTable state change on tooltip hide

Signed-off-by: Everett Ross <[email protected]>

* Add eslint rule to prevent unnecessary braces in jsx

Signed-off-by: Everett Ross <[email protected]>

* Add classname to tr to remove element selector, fix yarn.lock

Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>

Signed-off-by: vvvprabhakar <[email protected]>
yurishkuro added a commit that referenced this pull request Jun 17, 2023
## Which problem is this PR solving?
- Resolves #1503 

## Short description of the changes
- Adds a second Copy icon that copies value only
- Change Copy JSON icon to `snippets`, to be visually different
- Reduce fade-out time for tooltip

Original Copy JSON was added in #292 / #312.

---------

Signed-off-by: Yuri Shkuro <[email protected]>
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.

3 participants