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

Merged
merged 21 commits into from
Jan 16, 2025
Merged

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
@didip1000 didip1000 requested a review from unazko November 29, 2024 10:15
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.

packages/main/src/ColorPicker.ts Outdated Show resolved Hide resolved
packages/main/src/ColorPicker.ts Outdated Show resolved Hide resolved
packages/main/src/ColorValue.ts Outdated Show resolved Hide resolved
@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.

@unazko unazko requested a review from Stoev December 6, 2024 08:32
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

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

In the messagebundle.properties file please consider changing "Aria" to "ARIA".

packages/main/src/ColorValue.ts Outdated Show resolved Hide resolved
@didip1000 didip1000 requested a review from unazko December 23, 2024 15:33
@didip1000 didip1000 requested review from Stoev and unazko December 30, 2024 11:58
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

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

Agree to stick with original description.

@didip1000 didip1000 marked this pull request as ready for review January 16, 2025 14:05
@didip1000 didip1000 merged commit 8d0e5a4 into main Jan 16, 2025
10 checks passed
@didip1000 didip1000 deleted the colorpicker-hsl branch January 16, 2025 14:08
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
4 participants