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

fix(radio-button-group): add name and required props #1037

Merged
merged 3 commits into from
Oct 1, 2023

Conversation

brunnerh
Copy link
Contributor

@brunnerh brunnerh commented Jan 22, 2022

RadioButton will receive properties from group.

Fixes #1036.

Not the prettiest code, though.


setContext("RadioButtonGroup", {
selectedValue,
groupName: { subscribe: groupName.subscribe },
groupRequired: { subscribe: groupRequired.subscribe },
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

checked="{checked}"
disabled="{disabled}"
required="{required}"
required="{'required' in $$props ? required : $groupRequired}"
Copy link
Collaborator

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.

Suggested change
required="{'required' in $$props ? required : $groupRequired}"
required="{$groupRequired || required}"

Copy link
Contributor Author

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).

Copy link
Collaborator

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.

Copy link
Contributor Author

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 &quot;&quot; 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>

Copy link
Contributor Author

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.

Copy link
Collaborator

@metonym metonym left a 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
@metonym metonym changed the title Add name and required to RadioButtonGroup. fix(radio-button-group): add name and required props Oct 1, 2023
@metonym metonym merged commit 24e2a88 into carbon-design-system:master Oct 1, 2023
2 checks passed
@brunnerh brunnerh deleted the radio-group-required branch October 1, 2023 20:54
@metonym
Copy link
Collaborator

metonym commented Oct 13, 2023

Released in v0.81.0

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.

Add name and required to RadioButtonGroup?
2 participants