feat: add other as an option to specific fields #3031
Conversation
|
This works well, but I would just suggest one improvement. When we have the "other" option active, can we hide the other input and only show it when the user clicks on the other option in the form and then we display the input and automatically focus on it. |
|
I'm good with this implementation. @adrians5j or @Pavel910 this is ready for a code review. |
|
Checking. |
There was a problem hiding this comment.
Although the feature doesn't look like it, the PR is not trivial. A lot of stuff to go through and wrap my head around.
Let's start with a couple of comments I left and we'll do additional iterations if needed.
BTW something I also noticed while giving this new feature a try. This "Allow other" text should be a bit smaller:
| name={`${fieldId}Other`} | ||
| id={`${fieldId}Other`} | ||
| ref={otherInputRef} | ||
| disabled={!checked({ option: otherOption, value })} |
There was a problem hiding this comment.
Couldn't we simply render or not render the OtherInput, depending on the checked call result? A bit confused why we're using disabled here and then hiding the field if it's true (via above CSS).
| change({ option: otherOption, value, onChange }); | ||
| if (e.target.checked && otherInputRef.current) { | ||
| otherInputRef.current.disabled = false; | ||
| otherInputRef.current.focus(); |
There was a problem hiding this comment.
Would the following eliminate all these lines of code?
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/autofocus
Let's try that, but I believe my previous comment would also need to be applied in order for the autofocus to work.
| } | ||
| }; | ||
| }, | ||
| renderSettings({ form }) { | ||
| return ( | ||
| <Grid> | ||
| <Cell span={12}> | ||
| <OptionsList form={form} multiple /> | ||
| <OptionsList form={form} multiple otherOptionSwitch /> |
There was a problem hiding this comment.
| <OptionsList form={form} multiple otherOptionSwitch /> | |
| <OptionsList form={form} multiple otherOption /> |
Let's rename to otherOption.
|
@adrians5j |
|
@adrians5j |
neatbyte-vnobis
deleted the
form-builder-other-radio-and-checkbox-options
branch
Changes
Implement issue "Forms: Add “other” as an option to specific fields"
How Has This Been Tested?
Manual
Documentation
None