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): SAP Horizon theme implementation #4168

Merged
merged 11 commits into from
Nov 2, 2021

Conversation

ndeshev
Copy link
Contributor

@ndeshev ndeshev commented Oct 25, 2021

No description provided.

@ndeshev ndeshev marked this pull request as ready for review October 26, 2021 04:43
@elenastoyanovaa
Copy link
Contributor

  • Slider icon is strange, but as far as I remember it will be changed?
  • The dots in the sample overwrap the slider
    Screenshot 2021-10-27 at 15 09 42
  • Tickmarks color differs from the one in ui5. what is more, the rule: color: var(--_ui5_slider_tickmark_color); for .ui5-slider-tickmarks is never applied. The color comes from the gradient on top
  • In ui5 the border radius is 8px, here it is 12px
  • Width and height of handle is different from ui5 (32x24 as in ui5 is 26x26 in cozy)
  • the color of the progress .ui5-slider-progress differs from the one in ui5

left: -0.875rem;
top: -0.125rem;
top: -0.063rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

top: -0.063rem; :( hacky..

Copy link
Contributor Author

@ndeshev ndeshev Oct 28, 2021

Choose a reason for hiding this comment

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

Why? :) In UI5 the positioning is similar and also achieved with negative values.

Copy link
Contributor

Choose a reason for hiding this comment

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

because in ui5 all of the negative values are 1, 2px which result in 0.0625rem or 0.125rem. Yours result in something like 1,008 px which would eventually lead to different positioning on different devices/browsers. Please try to use something which results to a rounded value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will adjust the rems from 0.063 to 0.0625. However, no browser will interpret 1.008px different from 1px :)

width: 0.375rem;
height: 0.375rem;
right: -0.875rem;
top: -0.063rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

:(

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.

Changing to compact breaks the handles layout

@ndeshev
Copy link
Contributor Author

ndeshev commented Oct 28, 2021

  • Slider icon is strange, but as far as I remember it will be changed?
  • The dots in the sample overwrap the slider
    Screenshot 2021-10-27 at 15 09 42
  • Tickmarks color differs from the one in ui5. what is more, the rule: color: var(--_ui5_slider_tickmark_color); for .ui5-slider-tickmarks is never applied. The color comes from the gradient on top
  • In ui5 the border radius is 8px, here it is 12px
  • Width and height of handle is different from ui5 (32x24 as in ui5 is 26x26 in cozy)
  • the color of the progress .ui5-slider-progress differs from the one in ui5
  • Yes, icons should be updated to the new ones
  • I fixed the examples, the 2 sliders had no space between them and overlapped so that the start point of the second appear like end point of the first and the opposite.
  • Tickmarks color are according to the specifications for Fiori 3 - #89919A which corresponds to the --sapField_BorderColor param. No delta specs are provided for Horizon so they should remain the same for now.
  • Fixed the unused color variable of the tickmarks.
  • Which border-radius? The handes' one is 8px. The tooltips ware with 12, I changed it to 8 to match the UI5 one as they were discussed with VDs. Also added a box-shadow, which is also not in the specifications, but approved by the designers.
  • For now I will not update the cozy handle metrics (dimensions), as we've discussed to leave it as they are at that stage of the theming.
  • I will upload a patch to fix the broken handles in compact density.
  • Updated the styles of the progress bar and it's container.

Copy link
Contributor

@niyap niyap left a comment

Choose a reason for hiding this comment

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

  1. The start point of ui5-slider component, background color is not correct.
  2. While moving the whole progress bar of ui5-range-slider, the handles should have "active" styling, not as hovered.
  3. Handles in compact mode look really small and ugly.
  4. In compact mode the offset between the focus outline and the handles is too big.

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.

I noticed that many of the values are without parameters and this is strange especially for colors. Do we also use hardcoded colors in UI5? If we used parameters, maybe some of the things would come out of the box and there would be no need to overwrite them in eachc theme. If no parameters are available, ignore my comment.

packages/main/src/themes/SliderBase.css Outdated Show resolved Hide resolved
packages/main/src/themes/SliderBase.css Outdated Show resolved Hide resolved
packages/main/src/themes/SliderBase.css Outdated Show resolved Hide resolved
@ndeshev ndeshev merged commit a19159b into SAP:master Nov 2, 2021
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.

3 participants