Slider value unset#4173
Conversation
📝 WalkthroughWalkthroughAdds an unset state to ChangesSlider unset/clear feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant limel-slider
participant Translations.get
participant ChangeEvent
User->>limel-slider: click clear-button
limel-slider->>limel-slider: handleClear()
limel-slider->>limel-slider: enterUnsetState()
limel-slider->>ChangeEvent: emit change(NaN)
limel-slider->>Translations.get: resolve value-not-set / clear-value
limel-slider->>limel-slider: focus() input
User->>limel-slider: type value
limel-slider->>limel-slider: handleInput() clears isUnset
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4173/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/slider/slider.tsx`:
- Around line 103-110: The slider’s new language prop is not demonstrated in the
examples, so add a `language` example to the slider documentation/demo so users
can see how `Slider` uses `language` to localize accessible labels. Update the
slider example content alongside the `language` prop in `slider.tsx` (and any
related docs/demo entry) to show a non-default language and the resulting
translated labels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 276aaa4c-b5de-497f-9f43-9e9ec86cb3b2
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (14)
src/components/slider/examples/slider-basic.tsxsrc/components/slider/examples/slider-unset.tsxsrc/components/slider/partial-styles/_thumb.scsssrc/components/slider/slider.scsssrc/components/slider/slider.spec.tsxsrc/components/slider/slider.tsxsrc/translations/da.tssrc/translations/de.tssrc/translations/en.tssrc/translations/fi.tssrc/translations/fr.tssrc/translations/nl.tssrc/translations/no.tssrc/translations/sv.ts
Consolidated Review —
|
Kiarokh
force-pushed
the
slider-value-unset
branch
from
ad98145 to
ac93ecd
Compare
…lue-not-set` These are useful for various input-type components, specially to be used as `aria-label` or `aria-valuetext`
The slider was the only input in the library that could not be empty: it always resolved to a value and rendered "unset" identically to the minimum. It now supports an explicit unset state — the thumb rests centered with no fill, the indicator shows `?`, the label stays unfloated, and assistive technology announces "Value not set" through `aria-valuetext`. A trailing clear button (with a context-aware `aria-label` and a tooltip) lets users return to the unset state. Clearing emits `NaN` on the `change` event, keeping its payload a `number` so consumers are not forced into a breaking type change. Refs #4161 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the value was cleared, focus stayed on the clear button, which immediately disables itself — so focus fell to the document body. Move focus to the range input instead, so keyboard users can adjust the value right away and assistive technology announces the now-unset state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Behavioral specs for the new unset/clear functionality: a non-finite value renders the empty state (`?` indicator, "Value not set"), a finite value including 0 counts as set, the empty state ends on first interaction, the clear button's contextual label and its disabled and visibility rules (hidden for required and readonly sliders), and clearing emits `NaN`, enters the unset state, and moves focus to the slider. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`translate.get` indexed `allTranslations[language][key]` directly, so a `language` in the `Languages` type without translations threw a `TypeError` and crashed the render. It now falls back to English (then the raw key) for any unmapped language, and maps `nb` (Norwegian Bokmål) to the existing Norwegian (`no`) translations. Surfaced by the slider's new `language` prop via the composite example's runtime test. Refs #4162 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Kiarokh
force-pushed
the
slider-value-unset
branch
from
ac93ecd to
74e0ae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/slider/slider.spec.tsx`:
- Around line 1-181: The slider spec in limel-slider lacks coverage for the new
language prop, so add tests in the existing “unset state” and “clear button”
areas of slider.spec.tsx that pass language: 'sv' through setup and assert
translated strings for rangeInput(root).getAttribute('aria-valuetext') and
clearButton(root).getAttribute('aria-label'). Use the current translation
sources to verify the expected Swedish text, and keep the assertions alongside
the existing English-language checks so the behavior of the language prop is
covered.
In `@src/components/slider/slider.tsx`:
- Around line 109-113: The JSDoc on the Slider component’s value prop describes
null and undefined as valid unset inputs, but the public type on value is only
number. Update the documentation in slider.tsx to match the actual TypeScript
contract by either removing null and undefined from the comment or widening the
value prop type if those inputs are intended; keep the guidance around NaN as
the supported programmatic unset path and ensure the comment on the value
property remains consistent with the Slider API.
- Around line 146-166: `percentageClass` is being set to `undefined` by the
unset-state flow, but it is still declared as a plain `string` in `slider.tsx`.
Update the `@State()` declaration for `percentageClass` to allow `undefined`,
and make sure the assignments in `enterUnsetState` and `setPercentageClass`
remain consistent with that type so strict null checks pass.
In `@src/global/translations.ts`:
- Around line 27-28: Update the fallback comment in translations logic so it no
longer uses nb as an unmapped example, since the Languages mapping now aliases
it. Adjust the wording near the fallback handling to reference a truly unmapped
browser locale such as en-US, or remove the example entirely, and keep the
explanation aligned with the existing translation fallback behavior in the
related mappings and lookup code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb84f237-a839-495e-bb11-e6c8c9cbd042
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (15)
src/components/slider/examples/slider-basic.tsxsrc/components/slider/examples/slider-unset.tsxsrc/components/slider/partial-styles/_thumb.scsssrc/components/slider/slider.scsssrc/components/slider/slider.spec.tsxsrc/components/slider/slider.tsxsrc/global/translations.tssrc/translations/da.tssrc/translations/de.tssrc/translations/en.tssrc/translations/fi.tssrc/translations/fr.tssrc/translations/nl.tssrc/translations/no.tssrc/translations/sv.ts
| import { render, h } from '@stencil/vitest'; | ||
| import { vi } from 'vitest'; | ||
|
|
||
| describe('limel-slider — unset & clear', () => { | ||
| async function setup(props: Record<string, any> = {}) { | ||
| const { root, waitForChanges } = await render( | ||
| <limel-slider {...props}></limel-slider> | ||
| ); | ||
| await waitForChanges(); | ||
|
|
||
| return { root, waitForChanges }; | ||
| } | ||
|
|
||
| const indicator = (root: any): HTMLElement => | ||
| root.shadowRoot?.querySelector('.indicator'); | ||
| const rangeInput = (root: any): HTMLInputElement => | ||
| root.shadowRoot?.querySelector('input[type="range"]'); | ||
| const clearButton = (root: any): HTMLButtonElement => | ||
| root.shadowRoot?.querySelector('button.clear-button'); | ||
|
|
||
| describe('unset state', () => { | ||
| it('treats a non-finite value as unset', async () => { | ||
| const { root } = await setup({ | ||
| label: 'Priority', | ||
| value: Number.NaN, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
|
|
||
| expect(root.classList.contains('is-unset')).toBe(true); | ||
| expect(indicator(root).textContent).toBe('?'); | ||
| expect(rangeInput(root).getAttribute('aria-valuetext')).toBe( | ||
| 'Value not set' | ||
| ); | ||
| }); | ||
|
|
||
| it('rests the thumb at the midpoint while unset, not at the minimum', async () => { | ||
| const { root } = await setup({ | ||
| value: Number.NaN, | ||
| valuemin: 0, | ||
| valuemax: 10, | ||
| }); | ||
|
|
||
| // The native input sits at the midpoint (not the minimum), so both | ||
| // arrow-key directions are live and there is no jump on first use. | ||
| expect(rangeInput(root).value).toBe('5'); | ||
| }); | ||
|
|
||
| it('treats a finite value, including 0, as set', async () => { | ||
| const { root } = await setup({ | ||
| value: 0, | ||
| valuemin: -10, | ||
| valuemax: 10, | ||
| }); | ||
|
|
||
| expect(root.classList.contains('is-unset')).toBe(false); | ||
| expect(indicator(root).textContent).toBe('0'); | ||
| expect(rangeInput(root).getAttribute('aria-valuetext')).toBeNull(); | ||
| }); | ||
|
|
||
| it('becomes unset again when the value is reset to a non-finite number', async () => { | ||
| const { root, waitForChanges } = await setup({ | ||
| value: 3, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
| expect(root.classList.contains('is-unset')).toBe(false); | ||
|
|
||
| (root as any).value = Number.NaN; | ||
| await waitForChanges(); | ||
|
|
||
| expect(root.classList.contains('is-unset')).toBe(true); | ||
| expect(indicator(root).textContent).toBe('?'); | ||
| }); | ||
|
|
||
| it('leaves the unset state as soon as the user changes the value', async () => { | ||
| const { root, waitForChanges } = await setup({ | ||
| value: Number.NaN, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| step: 1, | ||
| }); | ||
| expect(root.classList.contains('is-unset')).toBe(true); | ||
|
|
||
| const input = rangeInput(root); | ||
| input.value = '3'; | ||
| input.dispatchEvent(new Event('input')); | ||
| await waitForChanges(); | ||
|
|
||
| expect(root.classList.contains('is-unset')).toBe(false); | ||
| expect(indicator(root).textContent).toBe('3'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('clear button', () => { | ||
| it('labels the button with the slider label, falling back to a generic label', async () => { | ||
| const withLabel = await setup({ label: 'Priority', value: 3 }); | ||
| expect(clearButton(withLabel.root).getAttribute('aria-label')).toBe( | ||
| 'Clear value of Priority' | ||
| ); | ||
|
|
||
| const withoutLabel = await setup({ value: 3 }); | ||
| expect( | ||
| clearButton(withoutLabel.root).getAttribute('aria-label') | ||
| ).toBe('Clear value'); | ||
| }); | ||
|
|
||
| it('disables the clear button while unset, and enables it once set', async () => { | ||
| const unset = await setup({ | ||
| label: 'Priority', | ||
| value: Number.NaN, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
| expect(clearButton(unset.root).hasAttribute('disabled')).toBe(true); | ||
|
|
||
| const set = await setup({ | ||
| label: 'Priority', | ||
| value: 3, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
| expect(clearButton(set.root).hasAttribute('disabled')).toBe(false); | ||
| }); | ||
|
|
||
| it('offers no clear button for required or readonly sliders', async () => { | ||
| const required = await setup({ | ||
| label: 'Priority', | ||
| required: true, | ||
| value: 3, | ||
| }); | ||
| expect(clearButton(required.root)).toBeNull(); | ||
|
|
||
| const readonly = await setup({ | ||
| label: 'Priority', | ||
| readonly: true, | ||
| value: 3, | ||
| }); | ||
| expect(clearButton(readonly.root)).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('clearing', () => { | ||
| it('emits change with NaN and enters the unset state', async () => { | ||
| const { root, waitForChanges } = await setup({ | ||
| label: 'Priority', | ||
| value: 3, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
|
|
||
| const details: number[] = []; | ||
| root.addEventListener('change', (event: CustomEvent<number>) => { | ||
| details.push(event.detail); | ||
| }); | ||
|
|
||
| clearButton(root).click(); | ||
| await waitForChanges(); | ||
|
|
||
| expect(details).toHaveLength(1); | ||
| expect(Number.isNaN(details[0])).toBe(true); | ||
| expect(root.classList.contains('is-unset')).toBe(true); | ||
| }); | ||
|
|
||
| it('moves focus to the slider input when cleared', async () => { | ||
| const { root, waitForChanges } = await setup({ | ||
| label: 'Priority', | ||
| value: 3, | ||
| valuemin: 1, | ||
| valuemax: 5, | ||
| }); | ||
|
|
||
| const focusSpy = vi.spyOn(rangeInput(root), 'focus'); | ||
|
|
||
| clearButton(root).click(); | ||
| await waitForChanges(); | ||
|
|
||
| expect(focusSpy).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the language prop's effect on translations.
The suite verifies aria-valuetext and clear-button labels only with the default English language. There's no test confirming that setting language to a non-default value produces translated strings (e.g., aria-valuetext changes from "Value not set" to the Swedish equivalent when language: 'sv'). This is a gap in coverage for the new language prop.
🧪 Suggested test
it('uses translated aria-valuetext when language is set', async () => {
const { root } = await setup({
value: Number.NaN,
valuemin: 1,
valuemax: 5,
language: 'sv',
});
// Replace with the actual Swedish translation from src/translations/sv.ts
const expected = 'Värde inte angivet';
expect(rangeInput(root).getAttribute('aria-valuetext')).toBe(expected);
});
it('uses translated clear-button label when language is set', async () => {
const { root } = await setup({
label: 'Priority',
value: 3,
language: 'sv',
});
// Replace with the actual Swedish translation
const expected = 'Rensa värde av Priority';
expect(clearButton(root).getAttribute('aria-label')).toBe(expected);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/slider/slider.spec.tsx` around lines 1 - 181, The slider spec
in limel-slider lacks coverage for the new language prop, so add tests in the
existing “unset state” and “clear button” areas of slider.spec.tsx that pass
language: 'sv' through setup and assert translated strings for
rangeInput(root).getAttribute('aria-valuetext') and
clearButton(root).getAttribute('aria-label'). Use the current translation
sources to verify the expected Swedish text, and keep the assertions alongside
the existing English-language checks so the behavior of the language prop is
covered.
| * The value of the input. When the value is `NaN`, `null`, `undefined`, | ||
| * or otherwise not a finite number, the slider is considered unset. | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public value: number; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
JSDoc mentions null and undefined as valid value inputs, but the prop is typed number.
NaN is the only type-safe way for a TypeScript consumer to unset the slider programmatically. null and undefined would cause type errors in strict mode. Consider either removing null/undefined from the JSDoc (keeping NaN as the documented method) or widening the type to number | undefined.
📝 Suggested JSDoc clarification
/**
- * The value of the input. When the value is `NaN`, `null`, `undefined`,
- * or otherwise not a finite number, the slider is considered unset.
+ * The value of the input. When the value is `NaN` or otherwise not a
+ * finite number, the slider is considered unset. (At runtime, `null`
+ * and `undefined` also trigger the unset state, but `NaN` is the
+ * type-safe way to unset the slider programmatically.)
*/
`@Prop`({ reflect: true })
public value: number;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * The value of the input. When the value is `NaN`, `null`, `undefined`, | |
| * or otherwise not a finite number, the slider is considered unset. | |
| */ | |
| @Prop({ reflect: true }) | |
| public value: number; | |
| /** | |
| * The value of the input. When the value is `NaN` or otherwise not a | |
| * finite number, the slider is considered unset. (At runtime, `null` | |
| * and `undefined` also trigger the unset state, but `NaN` is the | |
| * type-safe way to unset the slider programmatically.) | |
| */ | |
| `@Prop`({ reflect: true }) | |
| public value: number; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/slider/slider.tsx` around lines 109 - 113, The JSDoc on the
Slider component’s value prop describes null and undefined as valid unset
inputs, but the public type on value is only number. Update the documentation in
slider.tsx to match the actual TypeScript contract by either removing null and
undefined from the comment or widening the value prop type if those inputs are
intended; keep the guidance around NaN as the supported programmatic unset path
and ensure the comment on the value property remains consistent with the Slider
API.
Source: Path instructions
| /** | ||
| * `true` while the slider has no value set. Kept separate from the `value` | ||
| * prop so that the unset state survives the brief window during dragging | ||
| * where the value has not yet been emitted back to the consumer. | ||
| */ | ||
| @State() | ||
| private isUnset: boolean; | ||
|
|
||
| private labelId: string; | ||
| private helperTextId: string; | ||
| private clearButtonId: string; | ||
| private inputElement?: HTMLInputElement; | ||
|
|
||
| public constructor() { | ||
| this.labelId = createRandomString(); | ||
| this.helperTextId = createRandomString(); | ||
| this.clearButtonId = createRandomString(); | ||
| } | ||
|
|
||
| public componentWillLoad() { | ||
| this.displayValue = this.multiplyByFactor(this.getValue()); | ||
| this.setPercentageClass(this.getValue()); | ||
| this.syncStateFromValue(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
percentageClass is typed string but assigned undefined in the new unset logic.
enterUnsetState (line 275) and setPercentageClass (line 429) both assign undefined to this.percentageClass, which is declared as @State() private percentageClass: string at line 141. This is a type error under strictNullChecks. Update the type to string | undefined.
🔧 Proposed type fix
`@State`()
- private percentageClass: string;
+ private percentageClass: string | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * `true` while the slider has no value set. Kept separate from the `value` | |
| * prop so that the unset state survives the brief window during dragging | |
| * where the value has not yet been emitted back to the consumer. | |
| */ | |
| @State() | |
| private isUnset: boolean; | |
| private labelId: string; | |
| private helperTextId: string; | |
| private clearButtonId: string; | |
| private inputElement?: HTMLInputElement; | |
| public constructor() { | |
| this.labelId = createRandomString(); | |
| this.helperTextId = createRandomString(); | |
| this.clearButtonId = createRandomString(); | |
| } | |
| public componentWillLoad() { | |
| this.displayValue = this.multiplyByFactor(this.getValue()); | |
| this.setPercentageClass(this.getValue()); | |
| this.syncStateFromValue(); | |
| `@State`() | |
| private percentageClass: string | undefined; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/slider/slider.tsx` around lines 146 - 166, `percentageClass`
is being set to `undefined` by the unset-state flow, but it is still declared as
a plain `string` in `slider.tsx`. Update the `@State()` declaration for
`percentageClass` to allow `undefined`, and make sure the assignments in
`enterUnsetState` and `setPercentageClass` remain consistent with that type so
strict null checks pass.
| // (e.g. a value that is in the `Languages` type but unmapped, such as | ||
| // `nb`), so a component never throws on an unsupported language. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Comment inaccurately cites nb as an unmapped example.
The comment says the fallback handles "a value that is in the Languages type but unmapped, such as nb", but nb is now aliased on line 17, so the fallback won't trigger for it. Consider updating the example to reference a browser-specific locale (e.g., en-US) or removing the nb reference.
📝 Suggested comment update
- // Fall back to English when the requested language has no translations
- // (e.g. a value that is in the `Languages` type but unmapped, such as
- // `nb`), so a component never throws on an unsupported language.
+ // Fall back to English when the requested language has no translations
+ // (e.g. a browser-specific locale like `en-US` that is not mapped),
+ // so a component never throws on an unsupported language.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // (e.g. a value that is in the `Languages` type but unmapped, such as | |
| // `nb`), so a component never throws on an unsupported language. | |
| // Fall back to English when the requested language has no translations | |
| // (e.g. a browser-specific locale like `en-US` that is not mapped), | |
| // so a component never throws on an unsupported language. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/global/translations.ts` around lines 27 - 28, Update the fallback comment
in translations logic so it no longer uses nb as an unmapped example, since the
Languages mapping now aliases it. Adjust the wording near the fallback handling
to reference a truly unmapped browser locale such as en-US, or remove the
example entirely, and keep the explanation aligned with the existing translation
fallback behavior in the related mappings and lookup code.
2 participants
fix #4161
Note
see the parent issue #4161 for more details and screen recordings before and after this change.
Summary by CodeRabbit
Summary by CodeRabbit
aria-valuetext.readonlynorrequired.Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: