Don't update saturation based on parsed color #536

bjarnef · 2023-07-18T21:27:22Z

Description

This PR fixes an issue in color picker component, when dragging the handle to the bottom, where it sometimes jump to bottom left, because when the color value of #000000 then const { h, s, l } = parsed.toHsl(); returns a saturation value of 0, which affect the gridHandleX to be 0.

#533

chrome_qFG9VWOgbD.mp4

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

How to test?

Screenshots (if appropriate)

Checklist

If my change requires a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.

github-actions · 2023-07-18T21:27:36Z

Hi there @bjarnef, thank you for this contribution! 👍

While we wait for the team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
The automated tests all pass (see "Checks" tab on this PR)
The level of security for this contribution is the same or improved
The level of performance for this contribution is the same or improved
Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

iOvergaard · 2023-07-19T09:29:26Z

Excellent, @bjarnef. Can we just cross saturation out like that for all scenarios? If yes, feel free to fix the linting errors and remove the console.log's and we'll get it merged 😎

bjarnef · 2023-07-19T11:00:49Z

@iOvergaard an issue with this is that saturation (and eventually hue as well) is not set the specific color value.
If commented hue the spectrum doesn't update, when hue slider value change.

It is fine that #000000 initially set the saturation to 0 and grid handle position at bottom left. We just don't want to update it while dragging the handle and the color value hit black color at bottom.

I think we may need some variables to keeper track of the grid handle coordinates, initially from color value and updated as handle is moved around, which update e.g. hue and saturation values, but doesn't affect current handle position Even though color is black at different positions.

However when a color from palette is selected it update the value property in <uui-color-area>, which update the hue, saturation etc. and the grid handle position based on that.

Another thing I noticed is that moving the handle using left/right arrow keys no longer seems to keep same Y-coordinate as it originally did https://shoelace.style/components/color-picker - it does in <uui-color-area> itself, but not when used in <uui-color-picker>

iOvergaard · 2023-07-24T08:08:33Z

Ok, @bjarnef, I think I see what you are getting at. I think we could try your proposal out by not updating the saturation. We can add additional variables to keep the handle coordinates in sync if needed down the road.

Would you like to try and get this PR to pass the automated tests?

bjarnef · 2023-07-24T08:29:01Z

@iOvergaard yeah, but not updating the saturation in value property means the spectrum isn't initially correct from color value set, or e.g. when hue slider change or a color from palette is selected.

It is also a similar issue with hue, e.g. with a green'ish gradient in color area, when dragging handle to bottom (black) it updates the value, which then parse the color and from #000000 hue and saturation value is zero, so the color area updates to a red'ish color spectrum.

chrome_VAGHi5Gj69.mp4

E.g. compare with how the Shoelace color picker handle this, where saturation and brightness are updated and events emitted:
https://github.com/shoelace-style/shoelace/blob/current/src/components/color-picker/color-picker.ts#L297-L305

but it also parsed color as hsva:
https://github.com/shoelace-style/shoelace/blob/current/src/components/color-picker/color-picker.ts#L576

where we here parse from hsla:

Umbraco.UI/packages/uui-color-area/lib/uui-color-area.element.ts

Line 261 in a4cec0d

`hsla(${hue}, ${saturation}%, ${lightness}%, ${alpha / 100})`

bjarnef · 2023-09-28T09:43:41Z

@iOvergaard I think it is related to this, but I noticed when color is black and changing the hue slider, the color area doesn't change - probably due how it parse the HSL values from #000000 (in color area component).
E.g. compare with this how color spectrum change, when color in black and hue is changing.
https://shoelace.style/components/color-picker

In the Shoelace color picker the values are modified directly, but here we have e.g. color area in an isolated component.
By default hue value from #000000 is zero, so we probably need a way to track this is use e.g. changed hue to 50, but selected color yet is #000000.

It also seems a change has been made and the picker icon is no longer shown here?
https://uui.umbraco.com/?path=/story/uui-color-picker--overview

bjarnef · 2023-09-28T14:21:31Z

@iOvergaard regarding change of hue slider, something weird happens in setColor() in color picker component.
originally the parameter was string, but now accept string or HslaColor.

Even though hue parsed in is greather than zero, the parse color using Colord instance return 0.

I added a workaround in 9fe8a29

chrome_nLnJEbp5c6.mp4

I also noticed something that has affected the opacity slider - when dragging the slider value, it shouldn't change opacify of the slider itself, only the preview, so it still show the color gradient like this: https://shoelace.style/components/color-picker#opacity

Fixed in dd34062 as well.

…d value itself may contain alpha

bjarnef · 2023-09-29T07:06:00Z

@iOvergaard can you review these changes? 👍

It solves a few issues.

When dragging handle to bottom (black), it doesn't just to bottom left. It also mean when picking black color it may not always set handle to bottom left, because or saturation but I think it is okay for now.
If alpha/transparency it set, the handle reflect this as in the Shoelace component.
Dragging opacify slider doesn't change opacify of slider itself as the color value may contain opacity - we need the color value without alpha.
Changing hue slider while color is black updates the color spectrum / color area as well.

It also noticed some issues with vertical slider mostly where click/drag to a position, sometimes the handle jumps back or move a few pixels afterwards, but we can looks at this in another PR.

iOvergaard · 2023-09-29T08:30:20Z

Hi @bjarnef, thanks for your updates and yes, we'll get it reviewed 👍

bjarnef · 2023-09-29T08:59:51Z

I think there may be a bug in Colord, when passing in HslaColor object to the Colord instance.
omgovich/colord#116

loivsen · 2023-09-29T09:11:11Z

Hey Bjarne, really great work! I have a few things that I have noticed looking at your color picker

When the color area is (for example) at a blueish tone and you drag the handle down, there is a slight color change of the color-area. See picture

The number 194 also appears twice?

Lastly, I noticed that the disabled option only disables the input itself, but you can still pick colors.

bjarnef · 2023-09-29T09:29:45Z

Hi @loivsen

Yes, I think it may be because of saturation: https://github.com/umbraco/Umbraco.UI/pull/536/files#diff-e26cfd40613cb37d73b2bf9ca447a92b2d7caac910979aa0dc992c7976494c0bR161

Maybe we could update it as well like with hue if value is not zero.

Regarding the duplicate 194 value, it was to output the hue in color picker (the other is inside hue slider). I will remove that.

bjarnef · 2023-10-02T06:21:59Z

Regarding the disabled feature its another issue, but in e.g. Codepen example of the Shoelace Color Picker, it looks something like this:
https://shoelace.style/components/color-picker

However it's still possible to toggle color format and pick a color via the eyedropper.

loivsen · 2023-10-11T13:34:19Z

I would like to get these improvements merged in. @bjarnef can you look into the build error and then get the branch up to date as well? 😊

bjarnef · 2023-10-12T08:54:34Z

Hi @loivsen

I will have a look at this.
Meanwhile you could have a look at #589 but I think <uui-popover> need some changes, so "on outside click" works on what's visually is outside popover container.

bjarnef · 2023-10-17T11:52:37Z

@iOvergaard I tried creating a Pen to use the color picker from UI library.
I looks pretty good, but I noticed the handle position at hue and opacity slider doesn't change. Is that expected?

<script type="module" src="https://cdn.jsdelivr.net/npm/@umbraco-ui/uui@latest/dist/uui.min.js"></script>

<uui-color-picker inline opacity>
</uui-color-picker>

chrome_PFTfloSGK8.mp4

bjarnef · 2023-10-17T12:09:36Z

packages/uui-color-area/lib/uui-color-area.element.ts

+          this.hue = h;
+        }
+
+        //this.saturation = s;


I tried the following here, but it seem it made the handle "jump" and bit anyway and the color spectrum seemed to be affected. When dragging the handle in the spectrum, I would expect the gradient shouldn't change.

if (s !== 0) { this.saturation= s; }

iOvergaard · 2023-10-19T13:15:09Z

@bjarnef that's a bit spooky, could you try with the latest RC to see if any related fixes are working?

The link is: https://cdn.jsdelivr.net/npm/@umbraco-ui/[email protected]/dist/uui.min.js

Something could also be happening in the CJS version, which your pen is using.

BTW @loivsen briefly mentioned that she thought it might not work in Firefox, did you notice if there are problems there?

bjarnef · 2023-10-23T06:27:56Z

@iOvergaard that didn't make a difference :)

<script type="module" src="https://cdn.jsdelivr.net/npm/@umbraco-ui/[email protected]/dist/uui.min.js"></script>

<uui-color-picker inline opacity>
</uui-color-picker>

It does seem the Color Picker works in Firefox as well at least when checking from https://uui.umbraco.com/

bjarnef · 2023-11-08T14:42:39Z

@iOvergaard do you want to have another look at this? or maybe @nielslyngsoe knows?

Although it don't quite fix this ensure issue, it also handle when hue and opacity change.
and a type in cssPropCurrentValue :)

Also any idea why the sliders doesn't work from codepen?

<script type="module" src="https://cdn.jsdelivr.net/npm/@umbraco-ui/uui@latest/dist/uui.min.js"></script>

<uui-color-picker inline opacity>
</uui-color-picker>

<script type="module" src="https://cdn.jsdelivr.net/npm/@umbraco-ui/[email protected]/dist/uui.min.js"></script>

<uui-color-picker inline opacity>
</uui-color-picker>

packages/uui-color-picker/lib/uui-color-picker.element.ts

bjarnef · 2024-02-04T13:14:21Z

@iOvergaard can we merge this for now and improve later? I think this work better than the current version, but there is space for improvement.

iOvergaard · 2024-02-05T08:00:01Z

@bjarnef sure thing... would you take a look at the failing build?

bjarnef · 2024-02-05T08:02:45Z

@iOvergaard it shows this, but it doesn't seem to be related to these changes.

iOvergaard · 2024-02-05T08:04:41Z

@bjarnef Storybook won't deploy from forks, but still the "Tests" job is failing as well as the linter.

packages/uui-color-picker/lib/uui-color-picker.element.ts

bjarnef · 2024-02-05T12:03:59Z

@iOvergaard the tests are happy now 😅

Don't update saturation based on parsed color

8397ad4

bjarnef mentioned this pull request Jul 18, 2023

[BUG] uui-color-area: Sometimes resets coordinates when selecting a color #533

Closed

iOvergaard added bug Something isn't working community/pr labels Jul 19, 2023

iOvergaard linked an issue Jul 19, 2023 that may be closed by this pull request

[BUG] uui-color-area: Sometimes resets coordinates when selecting a color #533

Closed

Merge branch 'v1/contrib' into bug/color-area-saturation

a716476

bjarnef added 2 commits September 28, 2023 10:46

Merge branch 'v1/contrib' into bug/color-area-saturation

87f951a

Workaround to update color area when hue slider change

9fe8a29

bjarnef added 8 commits September 28, 2023 16:40

Fix typo

35e7860

Don't change opacity of gradient in opacity-slider as value change an…

dd34062

…d value itself may contain alpha

Make handle opacity based on alpha

6468c60

Clear console

ea6af27

Clear console

2570972

Clear console

5eec6bd

Add comment

77774cb

Merge branch 'v1/contrib' into bug/color-area-saturation

75bfabf

Remove output of hue value

6d26f02

bjarnef and others added 4 commits October 14, 2023 14:16

Merge branch 'v1/contrib' into bug/color-area-saturation

a1dd812

Update saturation

03e3b72

Don't update saturation

3bdf47a

Merge branch 'v1/contrib' into bug/color-area-saturation

68b0de2

iOvergaard added the hacktoberfest/contrib-2023 label Oct 17, 2023

bjarnef commented Oct 17, 2023

View reviewed changes

Merge branch 'v1/contrib' into bug/color-area-saturation

06e324c

bjarnef mentioned this pull request Oct 19, 2023

V13: New login screen umbraco/Umbraco-CMS#14780

Merged

1 task

Merge branch 'v1/contrib' into bug/color-area-saturation

ade0a0f

bjarnef commented Nov 8, 2023

View reviewed changes

packages/uui-color-picker/lib/uui-color-picker.element.ts Show resolved Hide resolved

bjarnef mentioned this pull request Nov 15, 2023

uui-swatch: selection no longer works with space key #654

Closed

Merge branch 'v1/contrib' into bug/color-area-saturation

7d803bb

bjarnef commented Feb 5, 2024

View reviewed changes

packages/uui-color-picker/lib/uui-color-picker.element.ts Show resolved Hide resolved

bjarnef added 3 commits February 5, 2024 10:40

Remove commented line

429f299

Remove unused parameter

49e54c5

Run linter

f9598d5

iOvergaard approved these changes Feb 5, 2024

View reviewed changes

iOvergaard merged commit d83b8ba into umbraco:v1/contrib Feb 5, 2024
6 of 7 checks passed

bjarnef deleted the bug/color-area-saturation branch February 18, 2024 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't update saturation based on parsed color #536

Don't update saturation based on parsed color #536

bjarnef commented Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited by iOvergaard

Loading

iOvergaard commented Jul 19, 2023

bjarnef commented Jul 19, 2023 •

edited

Loading

iOvergaard commented Jul 24, 2023

bjarnef commented Jul 24, 2023 •

edited

Loading

bjarnef commented Sep 28, 2023 •

edited

Loading

bjarnef commented Sep 28, 2023 •

edited

Loading

bjarnef commented Sep 29, 2023 •

edited

Loading

iOvergaard commented Sep 29, 2023

bjarnef commented Sep 29, 2023

loivsen commented Sep 29, 2023

bjarnef commented Sep 29, 2023

bjarnef commented Oct 2, 2023

loivsen commented Oct 11, 2023

bjarnef commented Oct 12, 2023

bjarnef commented Oct 17, 2023

bjarnef Oct 17, 2023

iOvergaard commented Oct 19, 2023

bjarnef commented Oct 23, 2023 •

edited

Loading

bjarnef commented Nov 8, 2023 •

edited

Loading

bjarnef commented Feb 4, 2024

iOvergaard commented Feb 5, 2024

bjarnef commented Feb 5, 2024

iOvergaard commented Feb 5, 2024

bjarnef commented Feb 5, 2024

Don't update saturation based on parsed color #536

Don't update saturation based on parsed color #536

Conversation

bjarnef commented Jul 18, 2023 • edited Loading

Description

Types of changes

Motivation and context

How to test?

Screenshots (if appropriate)

Checklist

github-actions bot commented Jul 18, 2023 • edited by iOvergaard Loading

iOvergaard commented Jul 19, 2023

bjarnef commented Jul 19, 2023 • edited Loading

iOvergaard commented Jul 24, 2023

bjarnef commented Jul 24, 2023 • edited Loading

bjarnef commented Sep 28, 2023 • edited Loading

bjarnef commented Sep 28, 2023 • edited Loading

bjarnef commented Sep 29, 2023 • edited Loading

iOvergaard commented Sep 29, 2023

bjarnef commented Sep 29, 2023

loivsen commented Sep 29, 2023

bjarnef commented Sep 29, 2023

bjarnef commented Oct 2, 2023

loivsen commented Oct 11, 2023

bjarnef commented Oct 12, 2023

bjarnef commented Oct 17, 2023

bjarnef Oct 17, 2023

Choose a reason for hiding this comment

iOvergaard commented Oct 19, 2023

bjarnef commented Oct 23, 2023 • edited Loading

bjarnef commented Nov 8, 2023 • edited Loading

bjarnef commented Feb 4, 2024

iOvergaard commented Feb 5, 2024

bjarnef commented Feb 5, 2024

iOvergaard commented Feb 5, 2024

bjarnef commented Feb 5, 2024

bjarnef commented Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023 •

edited by iOvergaard

Loading

bjarnef commented Jul 19, 2023 •

edited

Loading

bjarnef commented Jul 24, 2023 •

edited

Loading

bjarnef commented Sep 28, 2023 •

edited

Loading

bjarnef commented Sep 28, 2023 •

edited

Loading

bjarnef commented Sep 29, 2023 •

edited

Loading

bjarnef commented Oct 23, 2023 •

edited

Loading

bjarnef commented Nov 8, 2023 •

edited

Loading