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-color-picker): add HSL color selection #10157

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

didip1000
Copy link
Contributor

@didip1000 didip1000 commented Nov 7, 2024

This change introduces the ability to select colors using HSL channels. Users can now toggle between viewing color values as RGB or HSL by selecting the ↔️ button.

colorpicker-3 (2)

Fixes: #10275

@didip1000 didip1000 self-assigned this Nov 7, 2024
Copy link
Contributor

@unazko unazko left a comment

Choose a reason for hiding this comment

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

There is the following bug:

  1. Change to HSL mode from the button
  2. Input 30 for H press Tab
  3. Input 20 for S press Enter
    Expected:
    The values in the sliders and in the gradient field are updated according to the lastly entered saturation value.

}

get colorChannelInputs() {
if (!this._displayHSL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could return on one line with a ternary operator.

}

this._setColor(tempColor);
this._setColor();
Copy link
Contributor

@unazko unazko Dec 2, 2024

Choose a reason for hiding this comment

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

It is not very clear to me which color gets set like this. The relation between the _colorValue internal property and _setColor is vague. It's more like we update the value property according to the current state of the _colorValue. Maybe the function name doesn't fit anymore.


set RGB(value: ColorRGB) {
this._rgb = value;
if (this._valid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When did this._valid got updated to correspond to the newly given RGB?

@didip1000
Copy link
Contributor Author

There is the following bug:

  1. Change to HSL mode from the button
  2. Input 30 for H press Tab
  3. Input 20 for S press Enter
    Expected:
    The values in the sliders and in the color pane gets updated.

If your starting color is HSL 0, 0, 100, Changing the hue from the input should update the hue slider to around orange, but since the light is still 100, no matter what you set the saturation or hue to, the color will still be white, so the main color picker wont get updated.

@unazko
Copy link
Contributor

unazko commented Dec 2, 2024

If your starting color is HSL 0, 0, 100, Changing the hue from the input should update the hue slider to around orange, but since the light is still 100, no matter what you set the saturation or hue to, the color will still be white, so the main color picker wont get updated.

  • You're right HSL 30, 0, 100 changes the hue slider and the gradient field to orange the first time. However if you change it again to 300 the hue slider and the gradient field should go pink. (I guess the main color also doesn't change with this interaction)
  • For the saturation part I get your point as well that the main color won't change. In the openui5 project in this case when you change the saturation the color picking cursor moves vertically in the gradient field (All into the white section with HSL 0, 0, 100). For example if you set HSL 0, 50, 100 then the color picker cursor gets vertically right in the middle of the gradient field.

My point is that even if the main color doesn't change those interactions from the openui5 project do reflect the input values with more detail.

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.

ui5-color-picker: Add HSL
2 participants