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.

Sign up for GitHub

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

Jump to bottom

chore(ClipboardCopy): updated tests #9574

Merged
merged 9 commits into from
Oct 30, 2023
Merged

chore(ClipboardCopy): updated tests #9574

merged 9 commits into from
Oct 30, 2023

Conversation

thatblindgeye
Copy link
Contributor

What: Closes #9532

Additional issues:

@thatblindgeye thatblindgeye changed the title Iss9532 clipboard copy update tests chore(ClipboardCopy): updated tests Sep 7, 2023
@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 7, 2023 •
edited
Loading

Preview: https://patternfly-react-pr-9574.surge.sh

A11y report: https://patternfly-react-pr-9574-a11y.surge.sh

@thatblindgeye thatblindgeye force-pushed the iss9532_clipboardCopy_update_tests branch from ea1243e to 3c7d4f4 Compare September 8, 2023 13:11
@thatblindgeye thatblindgeye force-pushed the iss9532_clipboardCopy_update_tests branch from 4c71132 to a71fe93 Compare October 9, 2023 17:12
@thatblindgeye thatblindgeye force-pushed the iss9532_clipboardCopy_update_tests branch from 476fa44 to e96b4bf Compare October 16, 2023 15:30
@thatblindgeye
Copy link
Contributor Author

I also updated how the aria-controls within ClipboardCopyToggle was rendered, as it should only be the single contentId instead of the concatenated id + contentId (more of a bug fix than anything but can revert if necessary)

wise-king-sullyman
wise-king-sullyman reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Awesome work on this so far, small handful of requsts/questions:

packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopy.test.tsx Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopy.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopy.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopy.test.tsx Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopyButton.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopyButton.test.tsx Outdated Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopyButton.test.tsx Show resolved Hide resolved
packages/react-core/src/components/ClipboardCopy/__tests__/ClipboardCopyToggle.test.tsx
const toggleButton = screen.getByRole('button');
expect(toggleButton).toHaveAttribute('aria-expanded', 'false');
});
expect(screen.getByRole('button')).toHaveAttribute('data-prop', 'test');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests for the default class and the className prop?

Copy link
Contributor Author

@thatblindgeye thatblindgeye Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tlabaj we had a question when discussing this. Currently the ClipboardCopyToggle component has className as part of the interface, but it's spread in the props object which is placed on our Button component internally. Do you happen to know if that was the intent (passing a className to ClipboardCopyToggle renders the class on pf-v5-c-button), or was the className meant to be passed on the div.pf-v5-c-clipboard-copy__toggle-icon"?

ClipboardCopyToggle file for reference

Copy link
Contributor

Choose a reason for hiding this comment

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

Can open up a new breaking change issue to spread the className div.pf-v5-c-clipboard-copy__toggle-icon. We may also consider adding a new prop to add a classname to the button.

@thatblindgeye thatblindgeye force-pushed the iss9532_clipboardCopy_update_tests branch from e96b4bf to 68d6f8f Compare October 19, 2023 12:25
wise-king-sullyman
wise-king-sullyman approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

🥳 looks great other than that last question about className

@thatblindgeye thatblindgeye mentioned this pull request Oct 30, 2023
Open
@tlabaj tlabaj merged commit 1195885 into patternfly:main Oct 30, 2023
10 checks passed
@thatblindgeye thatblindgeye mentioned this pull request Oct 30, 2023
Open
@patternfly-build
Copy link
Contributor

Your changes have been released in:

Thanks for your contribution! 🎉

tlabaj pushed a commit to tlabaj/patternfly-react that referenced this pull request Oct 31, 2023
@thatblindgeye
chore(ClipboardCopy): updated tests (patternfly#9574)
0c76314 
* chore(ClipboardCopy): updated tests

* Added tests for ClipboardCopy

* Removed generated tests, added snapshot

* Updated onTooltipHidden test to integration

* Separated tooltip and callback tests

* Adjusted wait time in cypress

* Updated snapshots after rebase

* Added tests for non-exported components

* Updated per Austin comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlabaj tlabaj tlabaj approved these changes

@wise-king-sullyman wise-king-sullyman wise-king-sullyman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clipboard copy - update unit tests
4 participants
@thatblindgeye @patternfly-build @wise-king-sullyman @tlabaj