-
-
Notifications
You must be signed in to change notification settings - Fork 262
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
fix(radio-button-group): add name
and required
props
#1037
fix(radio-button-group): add name
and required
props
#1037
Conversation
|
||
setContext("RadioButtonGroup", { | ||
selectedValue, | ||
groupName: { subscribe: groupName.subscribe }, | ||
groupRequired: { subscribe: groupRequired.subscribe }, |
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.
What's the reason for only assigning the subscribe method?
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.
Essentially turns the store into a readable, so it cannot be modified by descendants using the context. There's a utility function that could be used to do that, too.
src/RadioButton/RadioButton.svelte
Outdated
checked="{checked}" | ||
disabled="{disabled}" | ||
required="{required}" | ||
required="{'required' in $$props ? required : $groupRequired}" |
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.
I believe that both the name
and required
attributes should use the group value if set.
required="{'required' in $$props ? required : $groupRequired}" | |
required="{$groupRequired || required}" |
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.
In that case, should there maybe be a console warning?
To me it seems fairly intuitive that the child value would override the one "inherited" from the ancestor (though it does not make much sense to do so in the first place).
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.
I think having the group override is the behavior we want, since a radio group is defined by giving each of radio buttons in the group the same name.
I think we can omit the console warning in favor of docs – plus, the prop comment on RadioButtonGroup
is already fairly clear.
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.
Using ||
for fallback mechanism does not really work, e.g. if the group explicitly sets false
the individual option would be OR'd and essentially take precedence. I changed it so the properties on the group are just undefined
by default, so a more proper approach using ??
can be taken.
There also was an issue with how the fallback stores had a value which would stop reactive updates to name
/required
in RadioButton
instances without a group. Using the new mechanism this also works again now.
Here is some code I used for testing various configurations.
Code
<script>
import {
Button,
Checkbox,
DataTable,
FormGroup,
RadioButton,
RadioButtonGroup,
TextInput,
} from 'carbon-components-svelte';
import { onMount } from 'svelte';
let sg1 = undefined;
let g1 = 'group';
let g2 = 'group-2';
let g3 = 'group-3';
let ng1 = 'non-group';
let r1 = true;
let r2 = false;
let domData = [];
const updateDomState = () =>
setTimeout(
() =>
(domData = [
...document.querySelectorAll('input[type=radio]'),
].map((e) => ({
id: e.value,
value: e.value,
name: e.name,
required: e.required,
checked: e.checked,
})))
);
onMount(updateDomState);
</script>
<div style="display: grid; grid-template-columns: 1fr 1fr; gap: 1rem;">
<!-- svelte-ignore a11y-click-events-have-key-events a11y-no-static-element-interactions -->
<div on:input={updateDomState} on:click={updateDomState}>
<FormGroup legendText="Bind both, check selection">
<Checkbox bind:checked={r1} labelText="Required" />
<TextInput labelText="Group Name 1" bind:value={g1} />
<div>Selected: {sg1}</div>
<Button kind="ghost" size="small" on:click={() => (sg1 = undefined)}>Clear</Button>
<Button kind="ghost" size="small" on:click={() => (sg1 = 'one')}>one</Button>
<Button kind="ghost" size="small" on:click={() => (sg1 = 'two')}>two</Button>
<Button kind="ghost" size="small" on:click={() => (sg1 = 'three')}>three</Button>
<RadioButtonGroup name={g1} required={r1} bind:selected={sg1}>
<RadioButton value="one" labelText="one" />
<RadioButton value="two" labelText="two" checked />
<RadioButton value="three" labelText="three" />
</RadioButtonGroup>
</FormGroup>
<FormGroup legendText="Group name "" overrides individual">
<RadioButtonGroup name="">
<RadioButton value="four" labelText="four" name="four" />
<RadioButton value="five" labelText="five" />
<RadioButton value="six" labelText="six" />
</RadioButtonGroup>
</FormGroup>
<FormGroup legendText="Group required false overrides individual">
<RadioButtonGroup required={false}>
<RadioButton value="seven" labelText="seven" required />
<RadioButton value="eight" labelText="eight" />
<RadioButton value="nine" labelText="nine" />
</RadioButtonGroup>
</FormGroup>
<FormGroup legendText="Individual required = true">
<TextInput labelText="Group Name" bind:value={g2} />
<RadioButtonGroup>
<RadioButton value="ten" labelText="ten" name={g2} required />
<RadioButton value="eleven" labelText="eleven" name={g2} required />
<RadioButton value="twelve" labelText="twelve" name={g2} required />
</RadioButtonGroup>
</FormGroup>
<FormGroup legendText="Individual required = false">
<TextInput labelText="Group Name" bind:value={g3} />
<RadioButtonGroup>
<RadioButton value="thirteen" labelText="thirteen" name={g3} required={false} />
<RadioButton value="fourteen" labelText="fourteen" name={g3} required={false} />
<RadioButton value="fifteen" labelText="fifteen" name={g3} required={false} />
</RadioButtonGroup>
</FormGroup>
<FormGroup legendText="No RadioButtonGroup">
<Checkbox bind:checked={r2} labelText="Non-Group Required" />
<TextInput labelText="Non-Group Name" bind:value={ng1} />
<div>
<RadioButton value="sixteen" labelText="sixteen" name={ng1} required={r2} />
<RadioButton value="seventeen" labelText="seventeen" name={ng1} required={r2} />
<RadioButton value="eighteen" labelText="eighteen" name={ng1} required={r2} />
</div>
</FormGroup>
</div>
<div>
<h3>DOM State</h3>
<DataTable
size="compact"
useStaticWidth
headers={[
{ value: 'Radio button', key: 'value' },
{ value: 'name', key: 'name', display: v => JSON.stringify(v) },
{ value: 'required', key: 'required' },
{ value: 'checked', key: 'checked' },
]}
rows={domData} />
</div>
</div>
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.
Note that there is a minor breaking change: name
of RadioButton
is no longer set to an empty string by default.
This is more consistent with other inputs and results in a name
attribute not being added to the DOM by default.
This could be reverted without changing the main functionality.
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.
Left my feedback. This looks solid, otherwise, and is an important fix.
Properties will be inherited in `RadioButton` unless the respective property is set there. Fixes carbon-design-system#1036.
(Prettier removes trailing ` ` in Markdown docs => Use paragraph instead.)
…dioButton` - Fix reactivity when RadioButton is used without group - Use `readable` when writing is not required - Use `readonly` to make intent clearer. - Remove default values for `required` and `name` - More consistent with other input types - Means that `name` attribute will not be added for no reason
8bb4817
to
a81328f
Compare
name
and required
to RadioButtonGroup
.name
and required
props
Released in v0.81.0 |
RadioButton
will receive properties from group.Fixes #1036.
Not the prettiest code, though.