[space-picker] First iteration #143

DmitrySharabin · 2024-10-24T15:08:58Z

Partially addresses #123.

Usage examples

<space-picker></space-picker>

<space-picker value="oklab"></space-picker>

<space-picker spaces="oklch, p3, srgb"></space-picker>

<space-picker spaces="oklch, p3, srgb" value="p3"></space-picker>

<space-picker onspacechange="this.nextElementSibling.textContent = this.value"></space-picker>
<output></output>

netlify · 2024-10-24T15:09:16Z

✅ Deploy Preview for color-elements ready!

Name	Link
🔨 Latest commit	`42ae461`
🔍 Latest deploy log	https://app.netlify.com/sites/color-elements/deploys/6729f4419f15120008fb1c19
😎 Deploy Preview	https://deploy-preview-143--color-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

LeaVerou · 2024-10-24T15:35:58Z

src/space-picker/space-picker.js

+			default: () => Self.Color.spaces,
+			convert (value) {
+				// Drop non-existing spaces
+				let spaces = Object.fromEntries(Object.entries(value).filter(([id, space]) => space));


I don't think we should drop them, I think we should just not list them with a name. Just the id.

I.e. the name should default to the id.

Makes sense. Will fix it. Thanks!

LeaVerou · 2024-10-24T21:26:14Z

src/space-picker/space-picker.js

+
+	static formAssociated = {
+		like: el => el._el.picker,
+		role: "combobox",


Doesn't this default to the element we specified in like? If not, it probably should.

In our code, we try to default to computedRole. Unfortunately, it still isn't supported. It's probably under the flag in some browsers (not sure).

We should just use getComputedAccessibleNode() instead. It's not like we don't have access to this information at all.

(in Nude Element, in case that wasn't clear)

Got it. Thanks. I'll send a PR with the change.

LeaVerou · 2024-10-24T21:26:49Z

src/space-picker/space-picker.js

+		like: el => el._el.picker,
+		role: "combobox",
+		valueProp: "value",
+		changeEvent: "spacechange",


Shouldn't this use change?

src/space-picker/space-picker.js

LeaVerou · 2024-10-24T21:28:02Z

src/space-picker/space-picker.js

+
+				value += "";
+
+				return Self.Color.Space.get(value);


This is again the same issue with the string/object props. We need a prop that contains your actual input as a string (value) and one that contains the color space object (likely space or selectedSpace).

LeaVerou · 2024-10-24T21:29:13Z

src/space-picker/space-picker.js

+			this.value = this._el.picker.value;
+		}
+
+		this.dispatchEvent(new event.constructor(event.type, {...event}));


Doesn't defining the change event below take care of this?

DmitrySharabin · 2024-10-25T09:49:29Z

Thank you so much for your review! Every comment is on point, and I'll try to address it ASAP.

- Use `value` for actual input (as a string) and `selectedSpace` for the corresponding space object - Support custom grouping of color spaces via the `groups()` prop - Don't drop non-existent spaces; replace them with the `{ id, name: id }` objects - Expose the `valuechange` and `spacechange` events - Retarget the `input` event - In `formAssociated`, use `change` as the `changeEvent`

DmitrySharabin · 2024-10-30T17:09:56Z

@LeaVerou, I addressed all your feedback (thank you for it). Here are the changes I made:

Use value for actual input (as a string) and selectedSpace for the corresponding space object
Support custom grouping of color spaces via the groups() prop
Don't drop non-existent spaces; replace them with the { id, name: id } objects
Expose the valuechange and spacechange events
Retarget the input event
In formAssociated, use change as the changeEvent

Here is an example of using a custom grouping (from your color palettes research):

<space-picker id="space_picker" spaces="oklch, p3, srgb" value = "p3"></space-picker>

<script type="module">
	space_picker.groups = (spaces) => {
		let ret = {polar: {}, rectangular: {}};
		for (let id in spaces) {
			let space = spaces[id];

			if (space.id !== id) {
				continue;
			}

			let isPolar = space.coords.h?.type === "angle";
			ret[isPolar ? "polar" : "rectangular"][id] = space;
		}
		return ret;
	};
</script>

Could you please take a look at this PR one more time?

LeaVerou

Authors should not have to loop over spaces themselves in the grouping function, it doesn't seem like they get any benefit for the extra work, are there cases where the grouping depends on what other spaces are available? (and even if so, one can just check spaces?). I was thinking of something like a groupBy() function that just takes a single space as a param, and gets back a group name. groups could still exist as a computed prop.
Why do we need defaultGroups? That seems to exist purely for implementation convenience. No grouping is not just a single group, that implies a different UI.
We definitely need to pick up the design discussion around rawProps, the pattern keeps coming up.

DmitrySharabin · 2024-11-04T16:35:33Z

Thank you so much for your feedback! I tried to address all the issues you pointed out. Namely:

Add the groupBy() prop that expects a space and returns the name of a group the space belongs to

Providing grouping looks like so now:

<space-picker id="space_picker" spaces="oklch, p3, srgb" value = "p3"></space-picker>

<script type="module">
	space_picker.groupBy = (space) => {
		let isPolar = space.coords.h?.type === "angle";
		return isPolar? "Polar" : "Rectangular";
	};
</script>

Related to the previous one: groups is now a computed prop returning an object with all the groups
Use different UI for cases with grouping and without

I hope I got everything right. If not, I'll be happy to fix it.

* Add the `groupBy()` prop that expects a space and returns the name of a group the space belongs to * Related to the previous one: `groups` is now a computed prop returning an object with all the groups * Use different UI for cases with grouping and without Oops

LeaVerou

Looks good! Please add a README, and add it to the list of upcoming components.

DmitrySharabin · 2024-11-04T17:11:50Z

Looks good! Please add a README, and add it to the list of upcoming components.

Thank you! Will do.

DmitrySharabin · 2024-11-05T10:38:30Z

Done! It's alive and can be played with: https://elements.colorjs.io/src/space-picker/ 🎉

DmitrySharabin added 2 commits October 24, 2024 16:45

[space-picker] First iteration

24dd0a1

Fail gracefully when there are no “real“ color spaces provided

7bbb709

LeaVerou reviewed Oct 24, 2024

View reviewed changes

src/space-picker/space-picker.js Show resolved Hide resolved

LeaVerou reviewed Oct 24, 2024

View reviewed changes

DmitrySharabin force-pushed the space-picker branch from fad2aed to f416bd5 Compare October 30, 2024 17:01

DmitrySharabin requested a review from LeaVerou October 30, 2024 17:10

Formatting

28ca479

LeaVerou requested changes Nov 1, 2024

View reviewed changes

DmitrySharabin force-pushed the space-picker branch from 919edb7 to fe0d219 Compare November 4, 2024 16:29

DmitrySharabin requested a review from LeaVerou November 4, 2024 16:35

Address feedback

5ea258d

* Add the `groupBy()` prop that expects a space and returns the name of a group the space belongs to * Related to the previous one: `groups` is now a computed prop returning an object with all the groups * Use different UI for cases with grouping and without Oops

DmitrySharabin force-pushed the space-picker branch from fe0d219 to 5ea258d Compare November 4, 2024 16:47

LeaVerou approved these changes Nov 4, 2024

View reviewed changes

DmitrySharabin added 2 commits November 4, 2024 18:19

Add <space-picker> to the list of upcoming elements

9dea4f6

Add basic docs

42ae461

DmitrySharabin merged commit 1075f15 into main Nov 5, 2024
4 checks passed

DmitrySharabin deleted the space-picker branch November 5, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[space-picker] First iteration #143

[space-picker] First iteration #143

DmitrySharabin commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

LeaVerou Oct 24, 2024

LeaVerou Oct 24, 2024

DmitrySharabin Oct 24, 2024

LeaVerou Oct 24, 2024

DmitrySharabin Oct 30, 2024 •

edited

Loading

LeaVerou Nov 1, 2024

LeaVerou Nov 1, 2024

DmitrySharabin Nov 4, 2024

LeaVerou Oct 24, 2024

LeaVerou Oct 24, 2024

LeaVerou Oct 24, 2024

DmitrySharabin commented Oct 25, 2024

DmitrySharabin commented Oct 30, 2024

LeaVerou left a comment

DmitrySharabin commented Nov 4, 2024

LeaVerou left a comment

DmitrySharabin commented Nov 4, 2024

DmitrySharabin commented Nov 5, 2024

[space-picker] First iteration #143

[space-picker] First iteration #143

Conversation

DmitrySharabin commented Oct 24, 2024 • edited Loading

Usage examples

netlify bot commented Oct 24, 2024 • edited Loading

✅ Deploy Preview for color-elements ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitrySharabin Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitrySharabin commented Oct 25, 2024

DmitrySharabin commented Oct 30, 2024

LeaVerou left a comment

Choose a reason for hiding this comment

DmitrySharabin commented Nov 4, 2024

LeaVerou left a comment

Choose a reason for hiding this comment

DmitrySharabin commented Nov 4, 2024

DmitrySharabin commented Nov 5, 2024

DmitrySharabin commented Oct 24, 2024 •

edited

Loading

netlify bot commented Oct 24, 2024 •

edited

Loading

DmitrySharabin Oct 30, 2024 •

edited

Loading