-
Notifications
You must be signed in to change notification settings - Fork 358
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
feat(Radio): Convert radio to TypeScript #1945
Conversation
PatternFly-React preview: https://1945-pr-patternfly-react-patternfly.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #1945 +/- ##
==========================================
+ Coverage 82.33% 82.36% +0.03%
==========================================
Files 628 628
Lines 6980 6998 +18
Branches 136 143 +7
==========================================
+ Hits 5747 5764 +17
+ Misses 1161 1160 -1
- Partials 72 74 +2
Continue to review full report at Codecov.
|
@rebeccaalpert can you also add integration tests for this component as outlined on this README? |
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.
Should add integration tests for Radio and RadioProps.
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/Radio/Radio.tsx
Outdated
Show resolved
Hide resolved
Added demo/tests and addressed PR feedback. |
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.
Looks great. couple of comments.
@@ -0,0 +1,26 @@ | |||
describe('Radio Demo Test', () => { | |||
it('Navigate to demo section', () => { |
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.
Can you add some interactive test for checking and unchecking the controlled and uncontrolled buttons?
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.
It has one for the controlled button, but the uncontrolled buttons don't change at all HTML-wise when they're selected, so I don't know that you can write that kind of test for them.
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.
LGTM
Converted to TypeScript. I made a small change to the behavior since it was triggering a React warning related to switching from an uncontrolled to a controlled component (and back again) when you clicked the controlled radio and one about defaultChecked and checked being applied at the same time.
Fixes #1997.