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, ui5-range-slider): add input as a tooltip #9547

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

ndeshev
Copy link
Contributor

@ndeshev ndeshev commented Jul 23, 2024

Implement editable tooltips in ui5-slider and ui5-range-slider

packages/main/src/RangeSlider.hbs Outdated Show resolved Hide resolved
packages/main/src/themes/SliderBase.css Outdated Show resolved Hide resolved
packages/main/src/SliderBase.ts Outdated Show resolved Hide resolved
packages/main/src/SliderBase.ts Outdated Show resolved Hide resolved
packages/main/src/RangeSlider.ts Outdated Show resolved Hide resolved
@ndeshev ndeshev marked this pull request as ready for review August 19, 2024 06:47
@MapTo0
Copy link
Member

MapTo0 commented Aug 19, 2024

F2: should toggle focus to the handle to the input and back, now it traps in the input field
Live-change of the value in the input (e.g. by arrows) should not move the handle. Handle should be adjusted on change event (enter, focus out)
Tabbing goes to the tooltip when the tooltip is an input
Invalid values should trigger value state
Focus out when having invalid valud and focus in again => the tooltip has an empty value, should be the last valid one.
Something is wrong with the positioning of the handle itself, not its container, maybe remove left style?

@ndeshev
Copy link
Contributor Author

ndeshev commented Aug 20, 2024

We should discuss the arrow up/down behavior - ui5-input throws a change event in that case

Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

Slider

  • Change event is not fired by arrows, when focused on handle (maybe not be released with that change)
  • No events fired when value of the input field is changed

RangeSlider

  • go to Range Slider with steps, tooltips, tickmarks and labels

  • hold command and arrow right / left step is 4.4 ?

  • Events are messed as well (similar to Slider)

Overall:

  • lots of eslint-disable-line
  • try to add value and value states to be part of the state of the component
  • check static string IDs for aria refs
  • eventing is not working at all

packages/main/src/SliderBase.ts Outdated Show resolved Hide resolved
packages/main/src/SliderBase.ts Outdated Show resolved Hide resolved
this._isInputValueValid = parseFloat(tooltipInput.value) >= this.min && parseFloat(tooltipInput.value) <= this.max;

if (!this._isInputValueValid) {
tooltipInput.value = this._lastValidInputValue;
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to keep these in a state instead of calling custom setters?

Copy link
Contributor Author

@ndeshev ndeshev Sep 13, 2024

Choose a reason for hiding this comment

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

I managed to do it half-way in the ui5-slider but the ui5-input's value update on focusout alongside "Negative" value state behaves strange, so I had to keep one instance of a setter call in the component.

I did not succeed to do it in Range Slider at all, I spent a lot of time and effort trying to achieve it and I'm not sure that it is possible whatsoever with the current implementation of the sliders and the input, so I left the property settings in the code.

Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

Visually the input tooltip does not look like the one in UI5. The difference is even bigger in fiori_3 theme, where the input looks very strange.
Screenshot 2024-09-13 at 18 39 54
Screenshot 2024-09-13 at 18 40 10

packages/main/src/RangeSlider.hbs Outdated Show resolved Hide resolved
packages/main/src/RangeSlider.hbs Outdated Show resolved Hide resolved
packages/main/src/RangeSlider.ts Outdated Show resolved Hide resolved
packages/main/src/Slider.hbs Outdated Show resolved Hide resolved
packages/main/src/SliderBase.ts Show resolved Hide resolved
Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

  • .aria-describedby renders describedby even if there is no value. Please return undefined in the getter
  • where it is specified that the input of the slider receives the accessible name passed to the slider? I checked the acc spec and there is no such info for the inputs. I would expect that the elements with role slider will receive accessible-name/accessible-name-ref, not the input. The same is described in the ACC spec.
  • Currently you pass ui5-slider-accName even if ui5-slider-accName is empty, it should not be rendered as a span and also as an aria-label

packages/main/src/RangeSlider.ts Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@
@mouseover="{{_onmouseover}}"
@mouseout="{{_onmouseout}}"
@keydown="{{_onkeydown}}"
@keyup="{{_onkeyup}}"
@keyup="{{_onKeyupBase}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not call it _onkeyup in the base? Slider does not have a keyup so it will call the super class and RangleSlider has the keyup and it will call its keyup 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.

keyup has a parameter and the function is the base doesn't, the typescript linter is complaining, that's why i made 2 separate functions

packages/main/src/SliderBase.ts Show resolved Hide resolved
Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

still lots of disable-line, can we fix it?

Issue 1

  1. Open: http://localhost:8080/packages/main/test/pages/RangeSlider.html
  2. Go to Range Slider with steps, tooltips, tickmarks and labels
  3. Click on the second (12) handle's tooltip input
  4. Type 60 and Hit Enter
  5. 2 is returned?

issue 2

  1. Select any of the inputs
  2. Select all (command / control + a)
  3. Type - (to start typing e.g. -2)
  4. Value is clear?

issue 3

  1. Select any of the inputs
  2. Select all (command / control + a)
  3. Type 0 hit Enter
  4. tooltip disapers?

issue 4

  1. Open: http://localhost:8080/packages/main/test/pages/RangeSlider.html
  2. Select first input
  3. Type 14, hit Enter
  4. Type 16 hit Enter
  5. Value is not changed?

packages/main/src/RangeSlider.ts Outdated Show resolved Hide resolved
@ndeshev
Copy link
Contributor Author

ndeshev commented Sep 25, 2024

Issue 2 is a bug in the ui5-input with type 'number'.
The remaining es-lint disabled lines are not introduced with this PR.

@ndeshev
Copy link
Contributor Author

ndeshev commented Sep 26, 2024

Visually the input tooltip does not look like the one in UI5. The difference is even bigger in fiori_3 theme, where the input looks very strange. Screenshot 2024-09-13 at 18 39 54 Screenshot 2024-09-13 at 18 40 10

Tooltip inputs in UI5 are not following Fiori Visual Specifications

Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

Issues found during testing:

  • Focus tooltip with value 2
  • Change it to 12 hit Enter
  • Arrow Up + Enter
  • Arrow Down + Enter
  • Arrow down + Enter

Issue: Tooltip value is 11 but the slider valu is 12 for on of the tooltips

@ndeshev ndeshev requested a review from MapTo0 October 1, 2024 06:54
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.

6 participants