-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for oddcontrast ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
<style lang="scss"> | ||
@use 'config'; | ||
|
||
[data-field='color-gamut'] { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* 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
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
start.alpha = 1; | ||
set(end, channel, range[1]); | ||
end.alpha = 1; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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).
* main: upgrade stylelint Bump the dev-minor group with 11 updates upgrade all deps Bump the dev-major group with 3 updates
const throttled = $derived( | ||
SLIDERS[format].map(() => throttle(sliderGradient, 50)), | ||
); |
There was a problem hiding this comment.
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. 🤷
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