Umbraco.UI icon indicating copy to clipboard operation
Umbraco.UI copied to clipboard

Color picker

Open bjarnef opened this issue 2 years ago β€’ 34 comments

Follow-up on https://github.com/umbraco/Umbraco.UI/pull/112

Description

This PR add a Color Picker components splitted into smaller separate components. Furthermore it introduces a few utility functions like clamp.

// Clamp number between two values with the following line:
const clamp = (num, min, max) => Math.min(Math.max(num, min), max);

An alternatively version of clamp function can be found here: https://www.webtips.dev/webtips/javascript/how-to-clamp-numbers-in-javascript

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] 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)

https://user-images.githubusercontent.com/2919859/183895436-d6d77811-d376-40e4-905b-9fe86976e592.mp4

Checklist

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

TODO

  • [ ] Make vertical slider go from min at bottom to max at top.
  • [ ] Update swatches with displayValue or label.
  • [ ] Update handle position in color area when selecting color from palette.
  • [ ] Ensure background color change in opacity slider. Maybe we need to separate into uui-color-hue-slider and uui-color-opacity to make the markup a bit different as the opacity slider need a layer for transparency and a layer with background color. These could eventually share a ColorSliderMixin.
  • [x] Can we configure color picker as inline vs. popup, e.g. using popover component if not inline?
  • [x] Support transparent colors in swatches, e.g. rendered with checkered background?
  • [ ] Deselect color in palette, when color is changed via color area or via hue slider.
  • [x] Focus handle inside color area and color sliders when navigating using keyboard.
  • [x] Close popover using esc key.
  • [x] Color is copied when clicking the preview button, but should this has any focus/animation like in the Shoelace library? Furthermore it seems the inner button element doesn't fill full height of uui-button which is seen with a transparent color on hover/focus.
  • [ ] Can we use aria-label without a button text? It seems setting label on uui-button both set aria-label and button text.

bjarnef avatar Aug 03 '22 16:08 bjarnef

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
  • [x] πŸ’‘ 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 πŸ€– πŸ™‚

github-actions[bot] avatar Aug 03 '22 16:08 github-actions[bot]

@iOvergaard @nielslyngsoe @madsrasmussen I decided to go with the Colord library as it seems most lightweight yet flexible: https://colord.omgovich.ru/

bjarnef avatar Aug 08 '22 13:08 bjarnef

Very good, @bjarnef !

iOvergaard avatar Aug 08 '22 13:08 iOvergaard

@iOvergaard a bit more progress. In the uui-color-area I need to set color or hue value to update the spectrum as the hue value change.

<uui-color-area
          @change=${this.handleGridDrag}
          >
</uui-color-area>

Currently the color spectrum is generated based on this inside uui-color-area.

style=${styleMap({ backgroundColor: `hsl(${this.hue}deg, 100%, 50%)` })}

I tried something like this, but how would I get the updated value inside the uui-color-area as the hue/color change? Maybe using watch on the value property?

<uui-color-area
          .value=${live(this.value)}
          @change=${this.handleGridDrag}
          >
</uui-color-area>

Furthermore I wonder if the change event should return the color or maybe an object... it could be an instance of Colord object: https://github.com/umbraco/Umbraco.UI/pull/280/files#diff-e26cfd40613cb37d73b2bf9ca447a92b2d7caac910979aa0dc992c7976494c0bR154-R156

The Shoelace library has some internal function for e.g. watch and emit: https://github.com/shoelace-style/shoelace/blob/8ee811af587a3e28da577ce046ee658310763882/src/components/animation/animation.ts#L3-L4

Besides that I am not sure how the selectable mixins should be used to trigger a change in current selected color. I guess the color swatches would both be selectable (single choice) and readonly.

bjarnef avatar Aug 10 '22 12:08 bjarnef

@iOvergaard could you help a bit with my questions in https://github.com/umbraco/Umbraco.UI/pull/280#issuecomment-1210579049

bjarnef avatar Aug 16 '22 09:08 bjarnef

Hi @bjarnef

Sorry, yes. If you want to react on the property called "value", you could do so by making a getter/setter like get value() and a set value(newVal) function, or you could react to changes in the lifecycle update or updated callbacks. Does that help you in any way?

If you want to return an object like colord you could do good by embedding it into an object that we control, so that if we ever switch colord with something else in the future, we won't have to change the entire event structure.

iOvergaard avatar Aug 16 '22 10:08 iOvergaard

Okay, I did try something like that, but didn't quite get it to work. Maybe I missed something, so I will have another look.

Would you prefer the event return object with an instance of Colord or the specific properties like r, g, b etc.? Do you have a example of another component returning an object when an event occurs?

bjarnef avatar Aug 16 '22 10:08 bjarnef

@bjarnef The combobox for instance returns a UUIComboboxEvent with a few string properties taken from an internal change event. There are other examples as well through out the library. I don't know exactly what properties are useful to have from colord but it sounds like a good approach to pull out details from their object, so that we can more easily swap it in the future if the need arises.

iOvergaard avatar Aug 16 '22 12:08 iOvergaard

@iOvergaard it kinda works now, but not sure about the value property as when color is #fff hue will be 0.

https://user-images.githubusercontent.com/2919859/184881680-7dd3ddc4-8c07-4d50-b193-2ea9739e78ea.mp4

I don't think we want to change current hue value in this case, in this example the hue should still be 116.

image

bjarnef avatar Aug 16 '22 12:08 bjarnef

https://user-images.githubusercontent.com/2919859/184919861-a60d7bff-af02-452c-88e6-f49ad307ed49.mp4

Regarding using Selectable mixin in color swatches I had a look at table row: https://github.com/umbraco/Umbraco.UI/blob/v1/dev/packages/uui-table/lib/uui-table-row.element.ts

But I don't see how it return trigger an event, so e.g. uui-color-swatches can react on the change or the uui-color-picker? I do get the selected attribute on each color swatch on click though.

bjarnef avatar Aug 16 '22 15:08 bjarnef

Not sure what you mean exactly, @bjarnef. The Selectable mixin just listens to click events and then sends either a SELECTED or UNSELECTED event bubbling up through the stack so you'll need something high up in the stack to catch the events that will then forward the appropriate values down to the other components.

iOvergaard avatar Aug 17 '22 08:08 iOvergaard

@iOvergaard I was looking at the example here: https://github.com/umbraco/Umbraco.UI/blob/v1/dev/packages/uui-ref/lib/uui-ref.element.ts#L19-L21

Besides the styling of the selected attribute I am not sure where I get the value related to selected element. Furthermore can we control only at single element to be selected at the time? E.g. the color of the selected swatch. In the color picker we may not neccessary style the selected color swatch, but could be useful if it is used at icon picker in backoffice, if we want this part like it is today.

bjarnef avatar Aug 17 '22 09:08 bjarnef

@bjarnef Uhh the value of the selected element. It's not included in the custom event, however you should be able to find it on the composed path if you are not listening to each color individually: https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath

Does that help you in any way? Otherwise I might be able to get someone else to help you next week ☺️

iOvergaard avatar Aug 19 '22 13:08 iOvergaard

@iOvergaard I had a look at the uui-combobox and uui-combobox-list components. From what I can see these are currently only single pickers? I guess it would make sense it these both support single/multi values.

We probably need something similar in color swatches. In Color Picker it would most likely be a single selection, but there may be other use-cases where multi selection makes sense.

https://user-images.githubusercontent.com/2919859/185665818-2e5ba1d7-9bb1-46f4-99b2-8fe6bd349f1e.mp4

bjarnef avatar Aug 19 '22 16:08 bjarnef

https://github.com/umbraco/Umbraco.UI/pull/280/files/211f00a80d05433d0147c1a06190a2b2d52935a9..e206638269b17393138d0e2eabbc50ce520b3877 makes the convertion works from color names like, tomato, skyblue, lavender etc.

bjarnef avatar Aug 19 '22 17:08 bjarnef

@iOvergaard @nielslyngsoe I have updated this PR with a few things that needs to be done, but in general it's getting there 😎

It would be fantastic if some of you could try this out πŸ€žπŸ™Œ

bjarnef avatar Aug 20 '22 12:08 bjarnef

Hi @bjarnef We are planning to review this very soon, and as you pointed out it would be useful in the new backoffice.

iOvergaard avatar Aug 31 '22 08:08 iOvergaard

ColorArea Default color seems to be white (or perhaps none), however the picker shows in the red area on load. Preferably they should be the same/match from the beginning if possible

ColorPicker The color sliders changes the width depends on the text of the color format button The eyedropper button sometimes open previous windows tab (thus tabbing out of the browser) Mouseover on the color preview shows a square when there is transparency (is this on purpose?) Swatches I miss some kind of indication for "this color is chosen". Forexample a checkmark on the chosen swatch color. Idea: It could also be a setting you could turn on and off for the swatches if not everybody would want this..

loivsen avatar Sep 19 '22 07:09 loivsen

@loivsen yes, the default color is empty, but it is difficult to select an empty color in the color area. https://shoelace.style/components/color-picker

but maybe empty value should show transparent (checkered background) and no selection in color area like this current color picker in Umbraco: https://seballot.github.io/spectrum/

The format button should probably have a fixed or minimum width to ensure it doesn't change as format/text is changed (unless a monospaced font was used and same number of characters).

I haven't noticed the eyedropper open/changing window/tab, but it does allow to pick any color on screen - also outside browser window.

Regarding swatches the selected color currently has a selected attribute, so we could easily style this with a checkbox, but we need to update this it color later is changed to another color via input, sliders or color spectrum. In this case the (and if used next to icon picker as today) it would probably be a single picker, but if used later by package developer it might make sense to allow picking multiple colors.

Currently the swatches should only render if any swatches has been provided, so I guess it's a setting to disable the swatches, but it could be a separate setting.

bjarnef avatar Sep 19 '22 09:09 bjarnef

Hi @bjarnef, is there anything we can assist you with from HQ's side?

iOvergaard avatar Oct 10 '22 11:10 iOvergaard

@iOvergaard I noted a few things here https://github.com/umbraco/Umbraco.UI/pull/280#issue-1327540098 which would be great to look at.

I think we should consider if the active color swatches should be selected in color picker and configure the color swatches to allow single/multiple selections.

Furthermore as @loivsen mentioned https://github.com/umbraco/Umbraco.UI/pull/280#issuecomment-1250683770 when a transparent color is selected (the preview styling on uui-button looks a bit strange on focus).

We also need to change color in opacity slider as hue change, but I am not sure if the inner element in can be styled (e.g. via a property). Alternatively we may need to split these into two components (but it may be useful with a base ColorSlider mixin / component to inherit from, e.g. to get the basic functionality with min, max etc).

Maybe a empty color shouldn't show a selected color in the color area, but some color pickers tends to use "red" as default selection in the HSB-grid.

bjarnef avatar Oct 10 '22 13:10 bjarnef

@iOvergaard I have merged with latest changes, but I now get some errors regarding import of the utils here:

image

I updated this https://github.com/umbraco/Umbraco.UI/pull/280/commits/ce8380b897b62be29b73e8fd5782105788699804 similar to demandCustomElement and Timer under utils, but it seems it is missing something as it doesn't know the member clamp and drag on import.

bjarnef avatar Oct 11 '22 20:10 bjarnef

@iOvergaard do you know what is cause the issue here? Furthermore I noticed there is no a v1/contrib branch... should I target the PR that branch instead?

bjarnef avatar Oct 12 '22 14:10 bjarnef

Trying to look into it, @bjarnef. It's a bit odd. Seems like it doesn't register any of the new types unless explicitly building the uui-base package. But even then it complains.

iOvergaard avatar Oct 13 '22 13:10 iOvergaard

You can continue targeting v1/dev here. We want to use v1/contrib moving forward for external pull requests.

iOvergaard avatar Oct 13 '22 13:10 iOvergaard

@iOvergaard yes I could access e.g. demandCustomElement and Timer from uui-color-slider because these existed in node_modules folder here:

umbraco-ui\packages\uui-color-slider\node_modules\@umbraco-ui\uui-base\lib\utils

image

But clamp and math wasn't copied to this folder (I guess it failed before building/copying these).

bjarnef avatar Oct 13 '22 13:10 bjarnef

I figured it out, I think. There were some references to an older version of uui-base which resulted in the package being symlinked incorrectly.

I allowed myself to push to your branch - hope that's okay!

Ref 18097f1a58eb56338c1b5c31357c48ec83c3eeaf

iOvergaard avatar Oct 13 '22 13:10 iOvergaard

But clamp and math wasn't copied to this folder (I guess it failed before building/copying these).

Yeah, we are using npm workspaces here so it actually symlinks the folder ./node_modules/@umbraco-ui/uui-base to ./packages/uui-base but I guess it doesn't work well if you target another version than what uui-base currently claims to be (in this case 1.0.0-rc.3 --> 1.0.0).

Normally lerna will keep these up-to-date, but I guess since your library only exists in your fork, the lerna commands were never run in your copy when we released 1.0.0.

iOvergaard avatar Oct 13 '22 13:10 iOvergaard

@iOvergaard yes, it seems to work after your change in https://github.com/umbraco/Umbraco.UI/commit/18097f1a58eb56338c1b5c31357c48ec83c3eeaf

I thought I tried that, but maybe something else failed at that time.

bjarnef avatar Oct 14 '22 13:10 bjarnef

@iOvergaard one issue I have it when styling uui-button I get this on hover/focus:

image

image

I think the main issue is when no label is set (I have added height: 100% here):

image

I also noticed that setting label on uui-button is set both aria-label and button text, but sometimes one want a button (like this without label) but aria-label for accessibility.

Furthermore I have an inline mode using uui-popover, but it doesn't seem to work well with uui-color-picker inside it. Sometimes it also "jumps" a bit to the correct position.

https://user-images.githubusercontent.com/2919859/195874809-95de1add-b57f-4a1a-a582-5123aae1a468.mp4

bjarnef avatar Oct 14 '22 14:10 bjarnef