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

Show gamut in sliders #226

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Show gamut in sliders #226

wants to merge 13 commits into from

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Dec 10, 2024

Description

Adds gamut boundaries to the sliders.

Also removes opacity from color channel sliders, and adds a checkerboard background to the opacity slider.

Related Issue(s)

#225

To test

Change gamut option, play with sliders.

Show me

https://deploy-preview-226--oddcontrast.netlify.app/

Todo

  • Design- Add border to slider?
  • Design- Abstract pattern from SpaceSelect and GamutSelect?

Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for oddcontrast ready!

Name Link
🔨 Latest commit 03dd34f
🔍 Latest deploy log https://app.netlify.com/sites/oddcontrast/deploys/679bddd55692340008b3c600
😎 Deploy Preview https://deploy-preview-226--oddcontrast.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

<style lang="scss">
@use 'config';

[data-field='color-gamut'] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copy pasted from SpaceSelect- should we make this a pattern somewhere?

color,
channel,
range,
gamut,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is somewhat computationally slow. I toyed around with caching results, but the cache quickly grew to 12 Mbs in about 30 seconds of heavy usage 🫨 . I could likely do some more optimization if it is slow on slower machines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed that too. My first attempt would probably be to try simple debouncing -- I'm not sure we need these to update immediately while dragging a slider? Or possible use a Web Worker to do the inGamut checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a throttle, and it's improved. I'm tempted to try it in a web worker, but unsure if it's worth it. Ideally it would be buttery smooth... but that may not be realistic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is much better IMO. I might play with a Web Worker just as a tutorial sometime, but I think this is fine.

@jamesnw jamesnw requested a review from jgerigmeyer December 11, 2024 20:49
@jamesnw jamesnw linked an issue Dec 16, 2024 that may be closed by this pull request
* main:
  lint
  Bump the dev-minor group across 1 directory with 15 updates
  Bump jsdom from 25.0.1 to 26.0.0 in the dev-major group
  Bump the dev-minor group with 6 updates
  upgrade node and yarn
  Bump the dev-minor group with 6 updates
  Bump the dev-minor group with 7 updates
  Bump the dev-minor group with 12 updates
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this a bit confusing to use in practice, without clearer explanation in the UX. Is it obvious what the "Gamut" dropdown is used for? Should gamut change automatically when "Format" is changed? Since the gamut setting really only impacts the sliders, would it be clearer for that setting to be in closer proximity to the sliders themselves, instead of grouped with the Format selector? Is it even useful to offer the "Gamut" selector at all, when the color format is a narrow space?

color,
channel,
range,
gamut,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed that too. My first attempt would probably be to try simple debouncing -- I'm not sure we need these to update immediately while dragging a slider? Or possible use a Web Worker to do the inGamut checks?

Comment on lines +35 to +37
start.alpha = 1;
set(end, channel, range[1]);
end.alpha = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why we're forcing the alpha here -- is it required somehow? It seems helpful to me for the sliders to represent alpha, even if we're also showing the gamut limitations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't required, but it does mirror patterns I see in other color pickers- I checked the built in Apple one, and oklch.com. I'm fine going either way on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting! I'd be curious what @stacyk and @SondraE think as users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgerigmeyer @jamesnw If I remember right, we chatted about how to do this with Miriam, played with other pickers together, and kinda group-designed it. I liked how it worked/looked when I tried it just now (though I don't come to it impartially).

src/lib/utils.ts Show resolved Hide resolved
src/lib/utils.ts Outdated Show resolved Hide resolved
src/lib/stores.ts Show resolved Hide resolved
src/lib/stores.ts Outdated Show resolved Hide resolved
jamesnw and others added 5 commits January 28, 2025 16:38
* main:
  upgrade stylelint
  Bump the dev-minor group with 11 updates
  upgrade all deps
  Bump the dev-major group with 3 updates
Comment on lines +22 to +24
const throttled = $derived(
SLIDERS[format].map(() => throttle(sliderGradient, 50)),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesnw Without making this $derived, it wasn't re-calculating these fns on initial page load when the format is read from the has. This seems to fix it.

I also bumped the throttle back to 50, which still seems to have decent performance for me. 🤷

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

Successfully merging this pull request may close these issues.

Show gamut ranges on sliders
5 participants