wip(ui5-range-slider): refactor ui5-range-slider component#13316
wip(ui5-range-slider): refactor ui5-range-slider component#13316
Conversation
JIRA: BGSOFUIRILA-4268
|
🚀 Deployed on https://pr-13316--ui5-webcomponents-preview.netlify.app |
fix bugs, adapt tests
acc improvements related to #13348
|
There is an issue when you click on the slider track and both start and end handle get selected and you try to move the handles using Arrow Left and Arrow Right.
Most of the times the range does not move.
|
|
The range slider has unexpected behavior when configured in the following way: 
Even though the min and max values are set to 0 , step>=1 is creating one tickmark in the middle. I would expect no tickmark to be created in case min and max values match. |
| * <br><br> | ||
| * <b>Note:</b> The value should be between the <code>min</code> and <code>max</code> properties of the parent <code>ui5-slider</code>. | ||
| * @since 2.19.0 | ||
| * @public |
There was a problem hiding this comment.
The class SliderHandle is declared as private but the properties are declared as public. Why is that? We should set them private as well. The comment is valid for the rest of the properties
| * @private | ||
| */ | ||
| @property() | ||
| handleType: "start" | "end" | "single" = "single"; |
There was a problem hiding this comment.
Using enum is more clear than different strings.
| --_ui5_slider_handle_focused_tooltip_distance: var(--_ui5_slider_tooltip_bottom); | ||
| --_ui5_slider_handle_box_sizing: content-box; | ||
| --_ui5_slider_tooltip_border_box: content-box; | ||
| --_ui5_range_slider_handle_background: #FFF; |
There was a problem hiding this comment.
Can we map this #FFF color to a variable, so when the theme changes its value will adjust to the theme (e.g. --sapSlider_RangeHandleBackground). You can also set it to transparent instead of #FFF
| --_ui5_slider_scale_progress_height: 100%; | ||
| --_ui5_slider_scale_progress_border: none; | ||
| --_ui5_range_slider_progress_focus_display: none; | ||
| --_ui5_range_slider_progress_focus_top: -1.063rem; |
There was a problem hiding this comment.
The paddings like -1.063rem, -1.428rem , 1.375rem , 1.438rem, etc. look like magic numbers to me. Can you derive them from some variable/parameter.
| const pageX = ctor.getPageXValueFromEvent(e); | ||
| const tempValue = ctor.getValueFromInteraction(e, this._effectiveStep, this._effectiveMin, this._effectiveMax, this.getBoundingClientRect(), this.directionStart); | ||
| const isInRange = tempValue >= this.startValue && tempValue <= this.endValue; | ||
| const startHandle = this.shadowRoot!.querySelector("[ui5-slider-handle][handle-type='start']"); |
There was a problem hiding this comment.
You already have getters for _startHandle and _endHandle. You can use them here instead of creating new variables startHandle and endHandle
| _pressTargetAndAffectedValue(clientX: number, value: number) { | ||
| const startHandle = this.shadowRoot!.querySelector(".ui5-slider-handle--start")!; | ||
| const endHandle = this.shadowRoot!.querySelector(".ui5-slider-handle--end")!; | ||
| const startHandle = this.shadowRoot!.querySelector("[ui5-slider-handle][handle-type='start']")!; |
There was a problem hiding this comment.
Use the getters _startHandle and _endHandle instead of creating new constants
| * from <label for="..."> elements, accessibleName, and the handle description. | ||
| * @private | ||
| */ | ||
| get _ariaLabelStartHandle() { |
There was a problem hiding this comment.
_ariaLabelStartHandle and _ariaLabelEndHandle are identical and differ only how handleDescription constant is obtained. You can think of improving this code redundancy
| style={{ | ||
| "inset-inline-start": `clamp(0%, ${position}%, 100%)`, | ||
| }} | ||
| ></SliderHandle> | ||
|
|
||
| <SliderTooltip | ||
| open={this._tooltipsOpen} | ||
| value={this.tooltipStartValue} | ||
| valueState={this.tooltipStartValueState} | ||
| min={this.min} | ||
| max={this.max} | ||
| data-sap-ui-start-value | ||
| editable={this.editableTooltip} | ||
| followRef={this._startHandle} | ||
| onChange={this._onTooltipChange} | ||
| onKeyDown={this._onTooltipKeydown} | ||
| onFocusChange={this._onTooltipFocusChange} | ||
| onOpen={this._onTooltipOpen} | ||
| onInput={this._onTooltipInput} | ||
| > | ||
| </SliderTooltip> | ||
| </div> | ||
| <div class="ui5-slider-handle-container" style={this.styles.endHandle} part="handle-container"> | ||
| <div class="ui5-slider-handle ui5-slider-handle--end" | ||
| {startTooltip(slider)} | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const endHandle = (slider: RangeSlider) => { | ||
| const position = _handlePosition(slider.min, slider.max, slider.endValue); | ||
|
|
||
| return ( | ||
| <> | ||
| <SliderHandle | ||
| value={slider.endValue} | ||
| min={slider.min} | ||
| max={slider.max} | ||
| tabIndex={slider._tabIndex} | ||
| disabled={slider.disabled} | ||
| active={slider.rangePressed} | ||
| handleType="end" |
There was a problem hiding this comment.
Use enum for handleType
3 participants
JIRA: BGSOFUIRILA-4268