-
Notifications
You must be signed in to change notification settings - Fork 175
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
[jsx/Form.js] new radio component! #5846
Conversation
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.
last few things to fix up, then i think we're good!
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
@zaliqarosli thanks. I made changes from your suggestions. Ready for re-review. |
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
@zaliqarosli thanks added the changes and you're right the props.value is unnecessary. |
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.
will test in a second!
hey @maltheism i'm trying to test this and it looks to me that the new_profile implementation in the PR description is out of date. It looks to me that a selected radio button needs a "checked" attribute. I don't see this implemented in the component anywhere so i'm not sure how to define the handleRadio function. is there another way of setting the chosen radio button value? could you test this on your end and see what we're missing? |
Hi @zaliqarosli I didn't think of adding a this.props.checked. I can see that as being useful for showing a module with a default radio input already selected. See my latest commit and I also updated the example code above for the new_profile to show how it works. |
@zaliqarosli see latest commit. I made the RadioElement have a state value that's associated with what ends up being checked. This helps the inputs for the RadioElement refresh when one is clicked because the input checked becomes true with the others being false. |
@zaliqarosli I added the latest suggestions. |
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.
last changes! tested with these changes and it works beautifully. thanks @maltheism !
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
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.
🎉🎉 🎉 🎉
Co-Authored-By: Zaliqa <zaliqa.rosli@mcin.ca>
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.
Nice addition for field where values are enum
type.
Create radio button component.
Brief summary of changes
<RadioElement/>
Note: I kept the design simple for now.
Preview:
Links to related tickets (GitHub, Redmine, ...)
Testing instructions (if applicable)
Example replace modules/new_profile/jsx/NewProfileIndex.js with: