Umbraco.UI
Umbraco.UI copied to clipboard
Color picker
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
anduui-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 aColorSliderMixin
. - [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 settinglabel
onuui-button
both setaria-label
and button text.
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 π€ π
@iOvergaard @nielslyngsoe @madsrasmussen I decided to go with the Colord library as it seems most lightweight yet flexible: https://colord.omgovich.ru/
Very good, @bjarnef !
@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.
@iOvergaard could you help a bit with my questions in https://github.com/umbraco/Umbraco.UI/pull/280#issuecomment-1210579049
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.
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 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 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.
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.
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 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 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 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
https://github.com/umbraco/Umbraco.UI/pull/280/files/211f00a80d05433d0147c1a06190a2b2d52935a9..e206638269b17393138d0e2eabbc50ce520b3877 makes the convertion works from color names like, tomato
, skyblue
, lavender
etc.
@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 π€π
Hi @bjarnef We are planning to review this very soon, and as you pointed out it would be useful in the new backoffice.
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 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.
Hi @bjarnef, is there anything we can assist you with from HQ's side?
@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.
@iOvergaard I have merged with latest changes, but I now get some errors regarding import of the utils here:
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.
@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?
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.
You can continue targeting v1/dev here. We want to use v1/contrib moving forward for external pull requests.
@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
But clamp
and math
wasn't copied to this folder (I guess it failed before building/copying these).
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
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 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.
@iOvergaard one issue I have it when styling uui-button
I get this on hover/focus:
I think the main issue is when no label is set (I have added height: 100%
here):
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