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(hasCheckbox): rename hasCheck to hasCheckbox #8403

Merged
merged 5 commits into from Jan 5, 2023

Conversation

bonjo7
Copy link
Contributor

@bonjo7 bonjo7 commented Dec 1, 2022

Issue 8126

What
Updated reference from hasCheck to hasCheckbox

@tlabaj updated PR in reference to hasCheckbox

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Dec 1, 2022

@thatblindgeye thatblindgeye linked an issue Dec 6, 2022 that may be closed by this pull request
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates! Had some changes below, mainly regarding the update to the old hasChecks prop.

@bonjo7
Copy link
Contributor Author

bonjo7 commented Dec 12, 2022

Thanks for the feedback @thatblindgeye I updated based on the above, if you feel I missed anything feel free to give me a shout 😄

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Thanks for these updates! I had a comment below regarding the one of the jobs failing for this PR

Additionally regarding the initial commit message of fix(hasChange): updated reference to hasCheckbox, that could be updated to more closely match our commit message convention #8134 is a PR similar where a prop was renamed.

@bonjo7 bonjo7 changed the title fix(hasChange): updated reference to hasCheckbox chore(hasCheckbox): rename hasCheck to hasCheckbox Dec 13, 2022
@bonjo7
Copy link
Contributor Author

bonjo7 commented Dec 13, 2022

@thatblindgeye I updated as per above. I noticed the test_jest is failing so I took a look locally and I have managed to make some changes to the packages/react-core/src/components/Timestamp/__tests__/Timestamp.test.tsx file and have the tests passing locally.

I'm not sure how my changes may have effected these tests but the changes are along the lines of changing dates, as an example

old -> expect(screen.getByText('1/1/2022, 12:00:00 AM')).toBeInTheDocument();
new -> expect(screen.getByText('1/1/2022, 00:00:00')).toBeInTheDocument();

I did not push these changes as they may be outside the scope of this ticket and might be an issue with local times etc

Note: all tests seem to have passed since writing the above

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

@bonjo7 I reran the failed job and it's now passing! Thanks for mentioning that and for making these updates 🎉

@bonjo7
Copy link
Contributor Author

bonjo7 commented Dec 13, 2022

@bonjo7 I reran the failed job and it's now passing! Thanks for mentioning that and for making these updates 🎉

Ah perfect, thanks for the feedback/help, I finally got there in the end. 😁

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit ce551a3 into patternfly:v5 Jan 5, 2023
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.

Breaking change: update hasCheck to hasCheckbox
4 participants