-
Notifications
You must be signed in to change notification settings - Fork 270
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
fix(ui5-color-picker): add tooltip to sliders #10270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltips are present and with accurate values.
@@ -366,41 +366,43 @@ class ColorPicker extends UI5Element implements IFormInputElement { | |||
} | |||
|
|||
_setMainColor(hueValue: number) { | |||
if (hueValue <= 255) { | |||
const hueValueMod = hueValue * 4.251; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, assigning to a new variable (hueValueMod
) and using it throughout the function, is necessary when you need both the passed param (hueValue
) and the modified one (hueValueMod
) at the same time. But if you'll only use the modified one, the best would be to just have: hueValue = hueValue * 4.251;
and continue using hueValue
in the function as before.
@@ -366,41 +366,43 @@ class ColorPicker extends UI5Element implements IFormInputElement { | |||
} | |||
|
|||
_setMainColor(hueValue: number) { | |||
if (hueValue <= 255) { | |||
const hueValueMod = hueValue * 4.251; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this magic number existed before, but this refactoring is an opportunity to set it to a constant.
@@ -436,7 +438,7 @@ class ColorPicker extends UI5Element implements IFormInputElement { | |||
// and HSL format, the color will be parsed to RGB | |||
// 0 ≤ H < 360 | |||
// 4.251 because with 4.25 we get out of the colors range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is now obsolete and should be deleted not to cause confusion.
🎉 This PR is included in version v2.6.0-rc.0 🎉 The release is available on v2.6.0-rc.0 Your semantic-release bot 📦🚀 |
Now both of the sliders have tootip showing the value selected.
fixes: #10203