-
Notifications
You must be signed in to change notification settings - Fork 487
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 ability to copy node data from graph #312
Merged
tiffon
merged 7 commits into
jaegertracing:master
from
everett980:issue-304-ability-to-copy-node-data-from-graph
Jan 18, 2019
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e7025b4
* Refactor KV Table copy icon into standalone component
everett980 0afd7ac
Adhere to prettier rules
everett980 2301b49
Add <CopyIcon /> to drawNode and OpNode
everett980 046de99
Improve variable names for splitting CopyIcon into own file
everett980 c9dec86
Clean up CopyIcon
everett980 5269cc3
Fix KeyValuesTable test coverage
everett980 7a31a5f
Merge branch 'master' into issue-304-ability-to-copy-node-data-from-g…
tiffon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,11 +14,34 @@ | |
|
||
import React from 'react'; | ||
import { shallow } from 'enzyme'; | ||
import { Dropdown, Icon, Tooltip } from 'antd'; | ||
import { CopyToClipboard } from 'react-copy-to-clipboard'; | ||
import { Dropdown, Icon } from 'antd'; | ||
|
||
import CopyIcon from '../../../common/CopyIcon'; | ||
|
||
import KeyValuesTable, { LinkValue } from './KeyValuesTable'; | ||
|
||
describe('LinkValue', () => { | ||
const title = 'titleValue'; | ||
const href = 'hrefValue'; | ||
const childrenText = 'childrenTextValue'; | ||
const wrapper = shallow( | ||
<LinkValue href={href} title={title}> | ||
{childrenText} | ||
</LinkValue> | ||
); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I would typically recommend saving to a const, |
||
}); | ||
|
||
it('renders correct Icon', () => { | ||
expect(wrapper.find(Icon).hasClass('KeyValueTable--linkIcon')).toBe(true); | ||
expect(wrapper.find(Icon).prop('type')).toBe('export'); | ||
}); | ||
}); | ||
|
||
describe('<KeyValuesTable>', () => { | ||
let wrapper; | ||
|
||
|
@@ -96,49 +119,12 @@ describe('<KeyValuesTable>', () => { | |
).toBe('span.kind'); | ||
}); | ||
|
||
describe('CopyIcon', () => { | ||
const indexToCopy = 1; | ||
|
||
it('should render a Copy icon with <CopyToClipboard /> and <Tooltip /> for each data element', () => { | ||
const trs = wrapper.find('tr'); | ||
expect(trs.length).toBe(data.length); | ||
trs.forEach((tr, i) => { | ||
const copyColumn = tr.find('.KeyValueTable--copyColumn'); | ||
expect(copyColumn.find(CopyToClipboard).prop('text')).toBe(JSON.stringify(data[i], null, 2)); | ||
expect(copyColumn.find(Tooltip).length).toBe(1); | ||
expect(copyColumn.find({ type: 'copy' }).length).toBe(1); | ||
}); | ||
}); | ||
|
||
it('should add correct data entry to state when icon is clicked', () => { | ||
expect(wrapper.state().copiedRows.size).toBe(0); | ||
wrapper | ||
.find('tr') | ||
.at(indexToCopy) | ||
.find(Icon) | ||
.simulate('click'); | ||
expect(wrapper.state().copiedRows.size).toBe(1); | ||
expect(wrapper.state().copiedRows.has(data[indexToCopy])).toBe(true); | ||
}); | ||
|
||
it('should remove correct data entry to state when tooltip hides', () => { | ||
wrapper.setState({ copiedRows: new Set(data) }); | ||
wrapper | ||
.find('tr') | ||
.at(indexToCopy) | ||
.find(Tooltip) | ||
.prop('onVisibleChange')(false); | ||
expect(wrapper.state().copiedRows.size).toBe(data.length - 1); | ||
expect(wrapper.state().copiedRows.has(data[indexToCopy])).toBe(false); | ||
}); | ||
|
||
it('should render correct tooltip title for each row', () => { | ||
wrapper.setState({ copiedRows: new Set([data[indexToCopy]]) }); | ||
const tooltips = wrapper.find(Tooltip); | ||
tooltips.forEach((tooltip, i) => | ||
expect(tooltip.prop('title')).toBe(i === indexToCopy ? 'Copied' : 'Copy JSON') | ||
); | ||
expect.assertions(data.length); | ||
it('renders a <CopyIcon /> with correct copyText for each data element', () => { | ||
const copyIcons = wrapper.find(CopyIcon); | ||
expect(copyIcons.length).toBe(data.length); | ||
copyIcons.forEach((copyIcon, i) => { | ||
expect(copyIcon.prop('copyText')).toBe(JSON.stringify(data[i], null, 2)); | ||
expect(copyIcon.prop('tooltipTitle')).toBe('Copy JSON'); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is the use of
:not(...)
necessary? Would something like the following work?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.
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.
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.
Word.