Add ability to copy node data from graph #312
Conversation
Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #312 +/- ##
==========================================
+ Coverage 82.54% 82.55% +<.01%
==========================================
Files 140 141 +1
Lines 3151 3141 -10
Branches 651 649 -2
==========================================
- Hits 2601 2593 -8
+ Misses 439 438 -1
+ Partials 111 110 -1
Continue to review full report at Codecov.
|
|
@everett980 please follow the same guidelines for PR description as for commit messages (described in CONTRIBUTING) |
| .DiffNode--popover .DiffNode--copyIcon, | ||
| .DiffNode:not(:hover) .DiffNode--copyIcon { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
Is the use of :not(...) necessary? Would something like the following work?
.DiffNode .DiffNode--copyIcon { display: none; }
.DiffNode:hover .DiffNode--copyIcon { display: unset; }There was a problem hiding this comment.
is there a reason to avoid :not?
I started using to avoid defining multiple styles to achieve one goal (hiding something).
I only discovered when working on #310 when I was styling based on both hover state and focus state in: /packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/TraceDiffGraph.css
That seemed easier than defining multiple styles so I started applying to more places.
packages/jaeger-ui/src/components/TraceDiff/TraceDiffGraph/drawNode.css
Outdated
Show resolved
Hide resolved
| @@ -57,6 +59,9 @@ class DiffNode extends React.PureComponent<Props> { | |||
| </td> | |||
| <td className={`DiffNode--labelCell ${className}`}> | |||
| <strong>{service}</strong> | |||
| <span className="DiffNode--copyIcon"> | |||
There was a problem hiding this comment.
You migth be able to skip having a wrapping node if you add a className prop to the CopyIcon component?
There was a problem hiding this comment.
that is cleaner, added
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.css
Show resolved
Hide resolved
everett980
changed the title
Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
| it('renders as expected', () => { | ||
| expect(wrapper.find('a').prop('href')).toBe(href); | ||
| expect(wrapper.find('a').prop('title')).toBe(title); | ||
| expect(wrapper.find('a').text()).toMatch(/childrenText/); |
There was a problem hiding this comment.
Nit: I would typically recommend saving to a const, const anchor = wrapper.find(...)
* * Refactor KV Table copy icon into standalone component Signed-off-by: Everett Ross <reverett@uber.com> * Adhere to prettier rules Signed-off-by: Everett Ross <reverett@uber.com> * Add <CopyIcon /> to drawNode and OpNode Signed-off-by: Everett Ross <reverett@uber.com> * Improve variable names for splitting CopyIcon into own file Signed-off-by: Everett Ross <reverett@uber.com> * Clean up CopyIcon Signed-off-by: Everett Ross <reverett@uber.com> * Fix KeyValuesTable test coverage Signed-off-by: Everett Ross <reverett@uber.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
## 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 <ysh@meta.com>
Which problem is this PR solving?
Short description of the changes