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

feat(ui5-slider): Add Slider component #2349

Merged
merged 46 commits into from
Nov 18, 2020
Merged

feat(ui5-slider): Add Slider component #2349

merged 46 commits into from
Nov 18, 2020

Conversation

ndeshev
Copy link
Contributor

@ndeshev ndeshev commented Oct 13, 2020

Introduce ui5-slider component that represents a numerical range and a handle (grip, thumb). The purpose of the component is to enable visual selection of a value in a continuous numerical range by clicking on the progress bar or moving an adjustable handle.

<ui5-slider id="basic-slider" max="20" value="5"></ui5-slider>

Overview

ui5-slider extends the SliderBase base class. It contains most of the component's API as well as it's CSS and the core part of the template file:

Properties:

  • min - The minimum value of the slider range
  • max - The maximum value of the slider range
  • value - The current value of the slider
  • step - Determines the increments in which the slider will move
  • showTickmarks - Displays a visual divider between the step values
  • showToolTip - Determines if a tooltip should be displayed above the handle
  • labelInterval - Labels some or all of the tickmarks with their values.
  • disabled- Defines whether the Slider is in disabled state.

Events:

  • change - Fired when the value changes and the user has finished interacting with the slider.
  • input - Fired when the value changes due to user interaction that is not yet finished - during mouse/touch dragging.

ui5-slider extends the API above with its specific property - value - it determines the current numerical value of the slider,
its handle position and the proportions of the progress bar.

Main features:
The Slider allows users to:

  • define custom "min" and "max" values of the Slider, they can be positive or negative numbers, round or decimal ones.
  • define a custom step - the step is the number with which the Slider value is increased/decreased. It also can be a round number or a decimal one.
  • the value property is always kept within the boundaries of the min and max they are synchronized between each other, along with their visual UI representation.

Feature Request : #1245

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@d3xter666 d3xter666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What bothers me in this change is that it keeps a record of lots of previous values. For example _prevStepValue, _prevValue, _prevMin, _prevMax, etc.

This means that these need to be in sync and ensure to avoid ambiguity.

packages/main/src/Slider.hbs Outdated Show resolved Hide resolved
packages/main/src/Slider.hbs Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
@ndeshev ndeshev changed the title feat(ui5-slider): Slider base functionality (draft) feat(ui5-slider): Slider Oct 26, 2020
@ndeshev ndeshev changed the title feat(ui5-slider): Slider feat(ui5-slider): Add Slider component Oct 26, 2020
packages/main/src/Slider.js Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
@ndeshev ndeshev marked this pull request as ready for review October 28, 2020 07:39
packages/main/src/Slider.hbs Outdated Show resolved Hide resolved
packages/main/src/SliderBase.hbs Outdated Show resolved Hide resolved
packages/main/src/SliderBase.hbs Outdated Show resolved Hide resolved
packages/main/src/SliderBase.hbs Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good when interacting with the component, still:

  • Add tests

  • Check ACC
    screen reader and keyboard handling.

  • Check RTL
    If you set dir="rtl" to the page the labels, steps are mirrored but the value and the tooltip not, so you you are dragging towards 20, but the value is heading to -20.

  • Add playground sample

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I did not get the "Inactive slider with no steps and tooltip" sample, why it is not active, I can't see disabled or inactive attribute, could you explain the idea.

And, you have to add "defaultValue" to the "min" property, because if you remove the "min" attribute, the slider stops working.

@ndeshev
Copy link
Contributor Author

ndeshev commented Nov 2, 2020

Also, I did not get the "Inactive slider with no steps and tooltip" sample, why it is not active, I can't see disabled or inactive attribute, could you explain the idea.

And, you have to add "defaultValue" to the "min" property, because if you remove the "min" attribute, the slider stops working.

I added a default value of 0 for the min prop.

About the Sliders with no step value - we had a few discussions on the subject. Initially I wrote it the opposite way - when no step is set (or 0) the Slider had its maximum possible precision.

However, we decided that without a specific value for exact quantization we can not rely that an interaction will be always adequately reproducible, if necessary (for example in case of a support request). When no constant quantization of the current Slider value is provided, it is entirely calculated based of the position and movement of the cursor and the placement of the Slider.
And the problem is that within that calculation there are aside factors that randomize it and we can not control - screen resolution, mouse DPI etc.

Basically, step must be defined so the value representation to be more "deterministic", more like "exact science". This is the reason why the Sliders can not be used with step of 0 in UI5 classic.

But I have forgotten to update the documentation for the property though.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find few inline comments

packages/main/test/samples/Slider.sample.html Outdated Show resolved Hide resolved
packages/main/test/samples/Slider.sample.html Outdated Show resolved Hide resolved
packages/main/src/Slider.hbs Outdated Show resolved Hide resolved
packages/main/src/SliderBase.hbs Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments

packages/main/test/samples/Slider.sample.html Show resolved Hide resolved
packages/main/src/Slider.js Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
ndeshev and others added 7 commits November 14, 2020 11:18
Normalization of the min and max value is introduced. If the min
property is set to be more than the max one they are swapped.

Validation of the step property is improved. If a negative value is
provided it gets converted to its positive equivalent.
Warning are now being display if the step value is 0, a negative number
or not a valid floating number.

tickmarks property is renamed to showTickmarks

Some refactoring is performed according to the code reviews.
- Fix Slider paddings
- Fix Handle's vertical position for Fiori 3
- Labels are no longer created using .createTextNode, just stored as a
  strings
-- Other small test improvements and fixes
@ndeshev
Copy link
Contributor Author

ndeshev commented Nov 16, 2020

Hello,

The current progress state is:

  • Component's architecture, file structure and inheritance is implemented as discussed.
  • All discussed features and API are implemented and extensively tested manually
  • The code review comments changes are done, except for the ones that we have discussed why they won't be.
  • Theme styles are added according to the specifications, including HCW and HCB variants.
  • RTL support is implemented
  • Playground samples
  • Test page and unit tests - need code review
  • Accessibility and keyboard handling will be added on a later stage.
  • The branch is synced with the updated UI5 Web Components master branch.

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far! Very impressive given the complexity of this component. Keep it up!

A couple of high-level remarks:

  1. With UI5 Web Components we're striving to minimize state that needs to be maintained and functions that need to be called on state change such as "recalcualteThis", "buildThat", etc.... It's usually better to have get _tickmarks() instead of drawTickmarks(). Same for all other similar functions that calculate some state that is later used in the template. Making getters for all of these will make the code much easier, because there will be much less in 'onBeforeRendering`. It's fine to cache values and avoid recalculations, just preferable to use getters and access them directly from the template than to explicitly call "build"-methods.

  2. You can only change "value" from the code as this is the only one affected by the end user's interaction. You cannot change step, min and max. Again, use _effective getters.

  3. For the CSS bug I'd like to have a theme-independent workaround. For modern browsers should be very easy. For IE, I'll play with it once you create the CSS vars in some of the next patches and see how we can leverage it.

  4. Do not call addEventListener unless really necessary (and add a comment why this was needed, if you do). This should be done by the framework via the .hbs @ syntax. When the component is updated, the events will not be detached by lit-html and reattached every time unnecessarily.

  5. About the way you access the base class - it's up to you. I don't mind if you prefer to keep it as it is. Just be wary of the difference and consequences.

{{#*inline "handles"}}
<div class="ui5-slider-handle" style="{{styles.handle}}">
{{#if showTooltip}}
<div class="ui5-slider-tooltip" style="{{styles.tooltip}}">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tooltip is not in the static area. Was that discussed? Probably the tooltip can be cut if there are overflow: hidden elements on the page. I guess it's not the biggest issue and makes the implementation much simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. this was not discussed previously. It will be good if it is ok to refactor this in the near future.

packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/Slider.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Show resolved Hide resolved
const stepStr = String(step);
const tickmarkWidth = "1px";

// There is a CSS bug with the 'currentcolor' value of a CSS gradient that does not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Still, I would prefer to use CSS variables, even from within JS.

getComputedStyle(document.documentElement)
    .getPropertyValue('__ui5_slider_currentcolor_something....')

As for IE, we'll have to see how we can achieve the same with the ponyfill. So for now leave it but this should be refactored asap.

packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
@ndeshev
Copy link
Contributor Author

ndeshev commented Nov 17, 2020

Hello guys, thanks for the comments, I committed the changes for them.

Vladi thank you for your kind words.
I left the function creating the labels and made a getter that returns the created array for them as a cached ref, because I think the function is a bit heavy to be executed on every rerendering, plus the Sliders get rerendered a lot on user interactions.

I refactored the other similar instances as proposed. I will also apply these comments in the Range Slider pull-request, where applicable.

packages/main/debug.log Outdated Show resolved Hide resolved
packages/main/src/SliderBase.js Outdated Show resolved Hide resolved
let step = this.step;

if (step === 0) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean return step? As it is currently undefined if 0

Copy link
Contributor Author

@ndeshev ndeshev Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a mistake of mine, if 0 or a positive number this function is supposed return it. As it is now it doesn't make any difference in the Slider - everything is working fine, but in the Range Slider it affects the UI.

I fixed it in the RangeSlider pull request (it is based on this one to use this base class and the shared CSS), when we submit it it will be fixed - #2310

vladitasev
vladitasev previously approved these changes Nov 17, 2020
@MapTo0 MapTo0 merged commit 2b9008c into SAP:master Nov 18, 2020
ilhan007 pushed a commit that referenced this pull request Nov 19, 2020
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.

7 participants