feat(cc-range-selector): init component#1591
Conversation
c879094 to
c6a2353
Compare
|
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-range-selector/init/index.html. This preview will be deleted once this PR is closed. |
c6a2353 to
5f13c21
Compare
pdesoyres-cc
left a comment
There was a problem hiding this comment.
Impressive work!
I've played with the component in stories and
- I didn't find any bugs
- I like the design very much
- The selection state is really easy to understand
I like how the component is split (controller, sub components)
I left some questions.
Well done for this work on this tricky component.
florian-sanders-cc
left a comment
There was a problem hiding this comment.
Well that's a really impressive PR and component, great job @roberttran-cc 🙌
😍 What I really really like:
- the design of the components, they look fantastic 💅,
- all the different states and their corresponding styles,
- the technical choices (controller for the dragging part),
- comments & naming overall (though I do have small suggestions, I'm really fine with what you chose),
- the code overall, it's really clean, comprehensive and easy to read 🙌!
I do have a few questions / issues. You won't be surprised, they're mostly related to accessibility (low level components are always tough when it comes to accessibility).
Screen reader compat / UX
This is typically a case where we would need user testing because we're kind of building something very custom, drifting from established (imperfect) patterns (double thumb range inputs or 1 input/select for start and another one for the end value).
For sighted users, this component brings a (way) better UX: we are able to see the values in the middle + metadata about the options.
For blind users, there is no semantics to say you have to select a start value and then an end value. It all comes down to the label/legend of the form control which may not be enough.
There is also no way for blind users to know that after selecting a range, if they try to select a value outside the range, we ditch their selection and they have to select a new end value 🤔.
Also the checkbox semantics kind of means that if they wanted to, they could choose "monday" and "thursday" but not what's in between.
I think we need to see if we can add some hidden texts / announcements to mitigate these issues (it's usually not a great idea to rely on texts & announcements instead of semantics but I'm not sure we can do better here, we'll see).
Let's discuss and test with a screen reader together when / if you want 😉. Without user testing, I think we should stick with what you did and try to improve what we can (because it does bring better UX for other users).
Keyboard interactions
Overall, the keyboard interactions are simple (great job).
Some users may expect another pattern (arrow keys + enter / space to select) but I don't think it's a big of a deal for sighted users (for blind users it's another story).
I think we should have the same visual feedback when using the keyboard as we get when using the mouse (dotted blue borders):
Disabled state styling
Usually, we style disabled form controls with a darker background compared to the active state.
It seems that we're doing the opposite with this component (disabled = blank, active = grey).
This could be confusing (or not).
Dragging behaviour nitpick
If I end my dragging action on something else than a cc-range-selector-option (typically the arrows in between), I end up in a weird mixed state where my selection is pending but not applied:
20251031-122446.mp4
Custom option
I don't understand the usecase and what it does. This makes me think it's something you need for the tunnel but I wonder if it's really something this generic component should carry (could be a button outside that controls this component).
Since I'm missing the usecase, I might be wrong about this whole comment 😄
After lunch thought ( 😅 ): |
HeleneAmouzou
left a comment
There was a problem hiding this comment.
A huge GG for this massive work ! 👏
I have checked the commits, the stories and played with the sandbox. I don't have much to add to what others have already said, the components look great and even though the code was a little bit tricky sometimes, the comments you left really help ! 👍
About the stories, I was wondering if we should add a "Required" example in the stories as we did for other form components ?
About the subjects to discuss, I agree with all the decision you took.
3e33973 to
41a105d
Compare
🧐 Visual tests report for PR #1591The latest visual tests report is available. Please review the results.
3 components impacted
This comment was generated automatically by the Visual tests workflow. |
41a105d to
6c6e7da
Compare
|
Answers and questions for @florian-sanders-cc. Disabled state stylingObservation: Disabled = blank/white, Active = grey. Usually opposite (disabled = grey, active = white). Answer: Actually agree, I probably got lost when styling the component. Decision: update to match other components (will be merged in the initial commits). Dragging Behavior BugIssue: Ending drag on non-option element (arrow) leaves selection in pending/preview state without applying. Root cause: mouseup only handled on cc-range-selector-option elements. If mouseup fires on arrow or gap, selection never applied. Decision: will be fixed to improve UX, although this state matches the behaviour when you start a selection with a simple click. Custom Option UsecaseQuestion: What's the usecase? Seems tunnel-specific. Should it be external button? Answer: Use case: Provide "escape hatch" for users needing values outside preset range. Example scenarios:
Why inside component:
Alternative: Could be external, but breaks visual grouping. MiscFor the record, in single mode, we keep a radio input. Also, the 'same visual feedback when using the keyboard as we get when using the mouse (dotted blue borders)' feedback is not relevant anymore I guess. QuestionWith the input range implementation, should we keep the error story in range mode? I disabled the a11y test on this one to prevent error. |
2ee95d5 to
6921581
Compare
6921581 to
9108a90
Compare
There was a problem hiding this comment.
I will continue my review later on, however here are two related bugs that comes from the custom option.
Bug #1: Customize Button Stays Active After Drag
When you click "Customize" and then drag to select options, the customize button remains visually active if you release the mouse outside of an option (on an arrow, gap, or elsewhere in the fieldset).
Reproduction Steps
- Open the range selector with custom option enabled
- Make a selection (e.g., select options 2-4)
- Click the "Customize" button
- Button becomes visually active
- Selection is cleared - Start dragging from an option and drag across several options
- Release the mouse over an arrow icon or gap (not directly on an option)
- ❌ Bug: The customize button remains highlighted
- ❌ Bug: _isCustomOptionActive stays true even though new selection is applied
Expected Behavior
When a new selection is made via drag (regardless of where mouseup occurs), the customize button should become inactive.
Bug #2: Arrow Icons and Border-Radius Artifacts
When _isCustomOptionActive = true, there's a state mismatch between where the rendermethod sources its index data vs where selection state is determined.
The Mismatch:
- In render() (line 810):
// When custom is active, uses _lastKnowSelection for rendering indexes
const { start, end } = !this._isCustomOptionActive
? this._getSelectionIndexes() // ← Uses this.selection
: this._lastKnowSelection; // ← Uses cached old selection - In _isOptionSelected() (line 704):
// ALWAYS uses this._getSelectionIndexes() which reads this.selection
const { start, end } = this._getSelectionIndexes(); // ← Always uses this.selection - In _renderOption() (lines 892-894):
const isFirst = indexes.current === indexes.start; // ← From render() - uses _lastKnowSelection when custom active
const isLast = indexes.current === indexes.end; // ← From render() - uses _lastKnowSelection when custom active
const isSelected = this._isOptionSelected(value); // ← ALWAYS uses this.selection
Result: When custom is active:
- isFirst and isLast are calculated based on the old selection (_lastKnowSelection)
- isSelected is false because this.selection = null
- CSS classes like 'within-selection', 'range-continues-left', 'range-continues-right' are applied based on the old selection
- This removes border-radius on options that aren't actually selected
- Arrows might be positioned incorrectly
Reproduction Steps
- Open the range selector with custom option enabled
- Select a range (e.g., options 2-5)
- Click "Customize"
- Selection is cleared
- _isCustomOptionActive = true
- _lastKnowSelection stores {start: 2, end: 5}
- this.selection = null - Look at the UI carefully
- ❌ Bug: Options 2-5 might have modified border-radius (flattened on inner sides)
- ❌ Bug: The visual state doesn't match the actual selection state
This becomes more visible if you interact further or if the customize button stays active (Bug #1).
One of the fix would be to clear custom state in _onFieldsetMouseUp:
File: cc-range-selector.js:453-457
_onFieldsetMouseUp() {
if (this._dragCtrl.isDragging() && this._dragCtrl.getSize() > 0) {
this._applyRangeSelection();
// -> this._isCustomOptionActive = false;
}
}
This would ensure the custom button is deactivated when a drag ends anywhere in the fieldset, not just on options.
There was a problem hiding this comment.
Code is easy to read, there are unit tests, lots of comments! Well done @roberttran-cc for this tricky component.
I've got nothing more to say apart from a bug which may be for the same reason as the one raised above by @Galimede. It may be just another way to end with this illegal state.
We can end up with a range selection AND the Customize button on:
Kooha-2026-01-13-12-01-02.webm
Reproduce with:
- go to With custom option story
- click on
Customizebutton - click on second option
- click on
Customizebutton
22153cb to
1ae6830
Compare
Galimede
left a comment
There was a problem hiding this comment.
Hey Bob, thanks for fixing the bug it works fine. 🤗
I've completed my review and don't have much to say, this is well documented, has some tests, and component's nice, LGTM! 💪
1ae6830 to
625120f
Compare
florian-sanders-cc
left a comment
There was a problem hiding this comment.
Alright this component is really awesome, I'll say it again but I really like the UX when using it with a mouse and now I also really like it when using the keyboard, even with a screen reader!
Thanks a lot for splitting things into multi review commits, it made the review a lot easier. Also thanks for answering my questions & notes, I agree with all your answers 😉.
About the code: everything looks clean, documented and tested when relevant so great job!
As discussed in sync, we're almost there, we now only need to deal with:
- range input in single mode would make a tiny bit more sense than radio input. Functionnally it's the same but range input do bring the "range" semantics with them.
- we should:
- hide the part we see from screen readers (
aria-hidden="true") - add a section that provides the same visual info right after the last range input. In this section we would:
- add a text to explain that it's a "recap for {legend}",
- add a
ul > listructure with info about each option (option name - selected / not selected / disabled)
- hide the part we see from screen readers (
I promise it's the last change then we're good (great actually) for real! 😇
625120f to
3e1df77
Compare
florian-sanders-cc
left a comment
There was a problem hiding this comment.
All good @roberttran-cc thanks for your patience and hard work on this, the component is really great!! 😎
3e1df77 to
0dd4450
Compare
|
🔎 The preview has been automatically deleted. |
🧹 Visual Changes Report deletedThe report and its associated data have been deleted because this PR has been closed. This comment was generated automatically by the Visual Changes workflow. |
What does this PR do?
cc-range-selectorcomponentcc-range-selector-optionsub componentrange-selector-dragging-controllerto manage drag selection statetrimArrayutility function tosrc/lib/utils.jswith comprehensive testsCcRangeSelectEventtocommon.events.jsCcRangeSelectorSelectCustomevent for custom option selectioncc-range-selectorin the form demo sandboxHow to review?
What's to discuss
Feel free to challenge/comment on the following points:
cc-range-selector)mode,selection,showCustom{ startValue, endValue }) vs storing full array