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

[New WC] Color space dropdown #123

Open
DmitrySharabin opened this issue Oct 16, 2024 · 21 comments
Open

[New WC] Color space dropdown #123

DmitrySharabin opened this issue Oct 16, 2024 · 21 comments

Comments

@DmitrySharabin
Copy link
Member

This component automatically gets populated with all color spaces (or a subset, e.g., polar or perceptually uniform, or even a custom subset).

<color-chart>, <color-scale>, and <color-picker> can benefit from it by integrating it and providing an easy way to choose (and dynamically change) the color space. Right now, it requires a lot of plumbing.

@LeaVerou
Copy link
Member

LeaVerou commented Oct 16, 2024

Name brainstorming:

  • <color-space> (too generic? is it possible we may want to do something else with this name?)
  • <color-space-picker> (too long?)

Wrt specifying the spaces:

  1. I think a good MVP would be all color spaces
  2. Going slightly beyond MVP, specific ids would be a good escape hatch for making custom things possible
  3. Going beyond that, some way to filter by coordinate name and/or type? :has(h)?
    • And that paves the way for other filters too down the line, e.g. :from(lab) for all spaces that descend from lab
  4. Partial ids? E.g. h*, *h basically encompasses all polar spaces, *lab encompasses both lab and oklab

The component should do double duty and function as a label for the color space, with clear affordances for clicking to change it. An MVP would be to just use a <select> + font: inherit.

@DmitrySharabin
Copy link
Member Author

Name brainstorming:

  • <color-space> (too generic? is it possible we may want to do something else with this name?)
  • <color-space-picker> (too long?)

By analogy with <gamut-badge>, how about <space-picker>?

I already have a working MVP, so if we have a name, it would help.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Oct 23, 2024

Wrt specifying the spaces:

  1. I think a good MVP would be all color spaces
  2. Going slightly beyond MVP, specific ids would be a good escape hatch for making custom things possible

I came up with this MVP (I use SpacePicker here for brevity; we can change it easily for whatever we want):

import ColorElement from "../common/color-element.js";

const Self = class SpacePicker extends ColorElement {
	static tagName = "space-picker";
	static url = import.meta.url;
	static shadowStyle = true;
	static shadowTemplate = `<select id="picker" part="picker"></select>`;

	constructor () {
		super();

		this._el = {};
		this._el.picker = this.shadowRoot.querySelector("#picker");
	}

	connectedCallback () {
		super.connectedCallback?.();
		this._el.picker.addEventListener("change", this);
	}

	disconnectedCallback () {
		super.disconnectedCallback?.();
		this._el.picker.removeEventListener("change", this);
	}

	handleEvent (event) {
		if (event.type === "change" && event.target === this._el.picker) {
			this.value = this._el.picker.value;
		}

		this.dispatchEvent(new event.constructor(event.type, {...event}));
	}

	propChangedCallback ({name, prop, detail: change}) {
		if (name === "spaces") {
			this._el.picker.innerHTML = Object.entries(this.spaces)
				.map(([id, space]) => `<option value="${id}">${space.name}</option>`)
				.join("\n");
		}

		if (name === "value" || name === "spaces") {
			if (this.value?.id !== this._el.picker.value) {
				if (this.value.id in this.spaces) {
					this._el.picker.value = this.value.id;
				}
				else {
					let currentValue = this.spaces[this._el.picker.value];
					console.warn(`No color space with id = “${ this.value.id }” was found among the specified ones. Using the current one (${ currentValue.id }) instead.`);
					this.value = currentValue;
				}
			}
		}
	}

	static props = {
		value: {
			default () {
				return Self.Color.Space.get(this._el.picker.value);
			},
			parse (value) {
				if (value instanceof Self.Color.Space || value === null || value === undefined) {
					return value;
				}

				value += "";

				return Self.Color.Space.get(value);
			},
			stringify (value) {
				return value?.id;
			},
		},

		spaces: {
			type: {
				is: Object,
				get values () {
					return Self.Color.Space;
				},
				defaultValue: (id, index) => {
					try {
						return Self.Color.Space.get(id);
					}
					catch (e) {
						console.error(e);
					}
				},
			},
			default: () => Self.Color.spaces,
			convert (value) {
				// Drop non-existing spaces
				return Object.fromEntries(Object.entries(value).filter(([id, space]) => space));
			},
			stringify (value) {
				return Object.entries(value).map(([id, space]) => id).join(", ");
			},
		},
	};

	static events = {
		change: {
			from () {
				return this._el.picker;
			},
		},
		spacechange: {
			propchange: "value",
		},
	};

	static formAssociated = {
		like: el => el._el.picker,
		role: "combobox",
		valueProp: "value",
		changeEvent: "spacechange",
	};
};

Self.define();

export default Self;
#picker {
	font: inherit;
	color: inherit;
	background: inherit;
	field-sizing: content;
	cursor: pointer;
}

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Oct 23, 2024

The spaces prop should probably be an array of color spaces (ids). Or maybe not. Hmm. 🤔 I'll try it tomorrow morning and see.

@DmitrySharabin
Copy link
Member Author

The spaces prop should probably be an array of color spaces (ids). Or maybe not. Hmm. 🤔 I'll try it tomorrow morning and see.

Nah, it shouldn't. Having an object is more convenient and is aligned with the Color.spaces object.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Oct 24, 2024

Here is a sneak peek of how it works:

Color.Space.Picker.mp4

I will send a PR once we decide on the component's name so we can iterate.

@LeaVerou
Copy link
Member

LeaVerou commented Oct 24, 2024

I think <space-picker> is fine. We can call the other one <channel-picker>, analogously to <channel-slider>. Though alternatively, we could adopt a naming scheme where everything starts with <color-*>, so <gamut-badge> will become <color-gamut-badge>, I'm undecided if the brevity is worth it.

This is fine for an MVP, but eventually I would like to also support some common groupings too, e.g. to show polar & rectangular color spaces in separate <optgroup>s, or perceptually uniform and not, as well as a custom grouping, provided via a JS function. Perhaps the MVP could just have the JS prop (and not reflect it).

@DmitrySharabin
Copy link
Member Author

This is fine for an MVP, but eventually I would like to also support some common groupings too, e.g. to show polar & rectangular color spaces in separate <optgroup>s, or perceptually uniform and not, as well as a custom grouping, provided via a JS function.

I was thinking of grouping, too (I examined what you did in your Color Palettes research), but I couldn't think of a default one, so I decided to let it go for now.

Perhaps the MVP could just have the JS prop (and not reflect it).

I want to make sure I get it right: the name of the prop is JS?

@LeaVerou
Copy link
Member

No lol, what would that mean? What I meant was, the prop would only exist in JS, i.e. it would not reflect. The prop name would probably be something like group or groups or groupBy.

@DmitrySharabin
Copy link
Member Author

No lol, what would that mean? What I meant was, the prop would only exist in JS, i.e. it would not reflect. The prop name would probably be something like group or groups or groupBy.

I thought so, but had to make sure 😂

@DmitrySharabin
Copy link
Member Author

MVP is alive and available for experiments: https://elements.colorjs.io/src/space-picker/

@sidewayss
Copy link

sidewayss commented Nov 19, 2024

I have two types of working homemade examples here: https://sidewayss.github.io/rAF/apps/color/

The two lists in the form are identical, the one on the right is a clone of the one on the left. The version inside the <color-picker> dialog is straight Colorjs, no CSS names (to open the dialog, click on the swatchy <div> at the right edge of the 2nd or 3rd row, labeled Start: and End:).

If you look at the code that loads the options into the lists:

  • it adds a custom space
  • then sorts the registry array again
  • then iterates to add the options

The ability to include custom spaces depends on the timing of populating the list, especially if you want to sort the list.

Also note that in the two primary lists, the value attribute of each <option> is not set to the Colorjs color space id. That's a detail that is specific to this app, but something else to consider in the design of this new element: are there options for the way it is populated? Otherwise my app's code would continue to roll its own because it couldn't use this new element, except inside the picker dialog.

re names: maybe <color-spaces> or <space-select> since the demo uses a <select>.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Nov 25, 2024

That's a detail that is specific to this app, but something else to consider in the design of this new element: are there options for the way it is populated?

I think it's a reasonable request. I also think <channel-picker> needs this feature, too.

To make custom things possible, I'd suggest adding a new JS-only prop to handle it, such as labelFor (spaceLabel, label, or something else).

It can be used, for example, like so:

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

<script>
    picker.labelFor = space => space.id;
</script>

As a result, we'll get:

image

Instead of the default:

image

Internally, this prop can be defined like so:

labelFor: {
	type: {
		is: Function,
		arguments: ["space"],
	},
	reflect: false,
},

Also note that in the two primary lists, the value attribute of each <option> is not set to the Colorjs color space id.

We can also do this, if there are use cases, the same way we can provide custom labels. E.g., via a valueFor(space) prop.

@LeaVerou, what do you think?

@sidewayss
Copy link

@DmitrySharabin - I understand your valueFor better than than labelFor. I like to start with concrete use cases, so here are a few you that can look at through the lens of your proposals:

  1. My app's color picker dialog has a list where the option text and value are both the spaceId. The MVP (what does that stand for here?) has the spaceId in the value and a longer descriptive name in the text. That's a simple pair with a clean feature difference.
  2. This previous comment mentions the idea of categories of spaces. I can imagine for example: hasHue || hasChroma, some kind of filtering mechanism by property that includes basic ||, &&, and ! logic. That would allow you to filter in/out rectangular spaces, for example. But it might be more work than it's worth if you provide solid instructions/examples for doing it yourself. An alternative would be presets for popular groupings of spaces, which would certainly be less work.
  3. The two main lists in my app have the option text and value set to something CSSy, if it's available. The idea is that if there's a CSS version of the color space, I want to use it. Else it's only available via Color.js. It also displays CSSy text because that's consistent with what it's doing. It even goes so far as to create a color space that uses rgb() with 0-255 as the range, distinct from color(srgb ...). On top of that, the option value for Color.js spaces is the serialized version, as in color(--space-id ...). I'm not sure what part(s) of that you would want to offer in a feature, or how that would work with @DmitrySharabin's proposals. I do think there's something universal about the idea of an option to use CSS names vs Color.js names, which vary in a ways I outlined here earlier this year.

@LeaVerou
Copy link
Member

I'm not sure wanting to display a CSS id is a common enough use case to warrant special syntax — I'd be inclined to go with what Dmitry suggested, and just called it getLabel or getSpaceLabel to clearly convey that it's a function.

Regarding things like hasHue || hasChroma, there is already a grouping prop that takes a function.

@sidewayss
Copy link

sidewayss commented Nov 25, 2024

I'm not sure wanting to display a CSS id is a common enough use case to warrant special syntax

Not just displaying, putting in the option's value, though maybe that should be outside the scope because it's about using CSS instead of Color.js. In other words, I don't disagree. The whole thing would be moot if Color.js space ids aligned with CSS :-)

Regarding things like hasHue || hasChroma, there is already a grouping prop that takes a function.

I now see that in the demo.

I was also hoping that maybe @DmitrySharabin could explain how his proposals would handle these cases, or not. I didn't understand his initial proposal for labelFor.

@sidewayss
Copy link

OK, I now see that labelFor is a property whose value is a function. So you can choose to set the <option> value or text to the id or whatever else. I think if you're going to go with <select> on the inside, then the name should be getSpaceText, not getSpaceLabel. You are using the HTMLOptionElement text or textContent property, not the label property, right? I'm not really sure what the label property is.

@LeaVerou
Copy link
Member

I'm not sure wanting to display a CSS id is a common enough use case to warrant special syntax

Not just displaying, putting in the option's value, though maybe that should be outside the scope because it's about using CSS instead of Color.js. In other words, I don't disagree. The whole thing would be moot if Color.js space ids aligned with CSS :-)

Yeah, this is out of scope. The picker gives you a ColorSpace object, it's trivial to convert values to whatever you want.

OK, I now see that labelFor is a property whose value is a function. So you can choose to set the <option> value or text to the id or whatever else. I think if you're going to go with <select> on the inside, then the name should be getSpaceText, not getSpaceLabel. You are using the HTMLOptionElement text or textContent property, not the label property, right? I'm not really sure what the label property is.

The label property/attribute is literally what an <option> shows. Its text is just used as a fallback if the label attribute is not set.

@sidewayss
Copy link

The label property/attribute is literally what an <option> shows. Its text is just used as a fallback if the label attribute is not set.

Wow, I've never seen it used, though I'm no expert. I don't see it being used in the demo page here either. I see textContent when I inspect the elements there, no label attribute. Regardless, it's just a name for a property here, and whatever is correct is correct. Thanks for bearing with my ignorance and my slowness in getting up to speed with this discussion.

@LeaVerou
Copy link
Member

Yup, at this point its convention to use the text content as the label. But in theory, it would be totally fine to do this: <option label="Adobe RGB Compatible">a98-rgb</option>. 🙃 See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/option#attributes

@sidewayss
Copy link

Yeah, this is out of scope. The picker gives you a ColorSpace object, it's trivial to convert values to whatever you want.

So you're saying that the value for these options is fixed by the element, but the user can vary the label? You're saying "no" to Dimitry's valueFor concept? Just to be clear.

Will this new element be used outside of any relationship with a <color-picker>? Or is it always effectively attached to one? I have a use case without a picker, but those lists are outside the scope of this element, so that fits.

Yup, at this point its convention to use the text content as the label.

That is what I thought. Yea, I read the MDN docs for label and linked to them in my comment above. I had just never seen it used and was puzzled by its use here in a property name.

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

No branches or pull requests

3 participants