-
-
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?
Changes from all commits
f90ce41
525812e
5c83b32
5188883
74eac4b
06bd0f4
fdecc3b
cbc0b9f
eb5aebd
488d656
4b1f277
7a77662
03dd34f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<script lang="ts"> | ||
import { GAMUTS } from '$lib/constants'; | ||
import { gamut } from '$lib/stores'; | ||
</script> | ||
|
||
<div data-field="color-gamut"> | ||
<label for="color-gamut" data-label>Gamut</label> | ||
<select name="color-gamut" id="color-gamut" bind:value={$gamut}> | ||
{#each GAMUTS as gamut (gamut.format)} | ||
{#if gamut} | ||
<option value={gamut.format}>{gamut.name}</option> | ||
{/if} | ||
{/each} | ||
</select> | ||
</div> | ||
|
||
<style lang="scss"> | ||
@use 'config'; | ||
|
||
[data-field='color-gamut'] { | ||
align-items: center; | ||
column-gap: var(--gutter); | ||
display: grid; | ||
grid-template: | ||
'format-label' auto | ||
'format-input' auto / 1fr; | ||
justify-content: end; | ||
|
||
@include config.above('sm-page-break') { | ||
grid-template: 'format-label format-input' auto / 1fr minmax(10rem, auto); | ||
} | ||
} | ||
|
||
label { | ||
grid-area: format-label; | ||
|
||
@include config.above('sm-page-break') { | ||
text-align: right; | ||
} | ||
} | ||
|
||
select { | ||
grid-area: format-input; | ||
} | ||
</style> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,10 @@ | ||
<script lang="ts"> | ||
import type { PlainColorObject } from 'colorjs.io/fn'; | ||
import throttle from 'lodash/throttle'; | ||
import type { Writable } from 'svelte/store'; | ||
|
||
import { type ColorFormatId, SLIDERS } from '$lib/constants'; | ||
import { ColorSpace } from '$lib/stores'; | ||
import { ColorSpace, gamut } from '$lib/stores'; | ||
import { getSpaceFromFormatId, sliderGradient } from '$lib/utils'; | ||
|
||
interface Props { | ||
|
@@ -16,11 +17,22 @@ | |
|
||
let targetSpace = $derived(getSpaceFromFormatId(format)); | ||
let spaceObject = $derived(ColorSpace.get(targetSpace)); | ||
|
||
// Create a throttled value for each channel | ||
const throttled = $derived( | ||
SLIDERS[format].map(() => throttle(sliderGradient, 50)), | ||
); | ||
Comment on lines
+22
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jamesnw Without making this I also bumped the throttle back to |
||
|
||
let sliders = $derived( | ||
SLIDERS[format].map((id) => { | ||
SLIDERS[format].map((id, index) => { | ||
const coord = spaceObject.coords[id]; | ||
const range = coord?.range ?? coord?.refRange ?? [0, 1]; | ||
const gradient = sliderGradient($color, id, range); | ||
const gradient = throttled[index]!({ | ||
color: $color, | ||
channel: id, | ||
range: range, | ||
gamut: $gamut, | ||
}); | ||
return { | ||
id, | ||
name: coord?.name ?? '', | ||
|
@@ -37,7 +49,12 @@ | |
); | ||
|
||
let alphaGradient = $derived( | ||
sliderGradient($color, 'alpha', [0, $color.alpha]), | ||
sliderGradient({ | ||
color: $color, | ||
channel: 'alpha', | ||
range: [0, $color.alpha], | ||
gamut: $gamut, | ||
}), | ||
); | ||
|
||
const handleInput = ( | ||
|
@@ -95,6 +112,7 @@ | |
style={`--stops: ${alphaGradient}`} | ||
value={$color.alpha} | ||
oninput={(e) => handleInput(e)} | ||
data-channel="alpha" | ||
/> | ||
</div> | ||
</div> | ||
|
@@ -105,6 +123,10 @@ | |
display: block; | ||
appearance: none; | ||
background: linear-gradient(to right, var(--stops)); | ||
&[data-channel='alpha'] { | ||
background: linear-gradient(to right, var(--stops)), | ||
url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 60 60"><rect fill="%23e8e8e8" width="30" height="30"/><rect x="30" y="30" width="30" height="30" fill="%23e8e8e8"/></svg>'); | ||
} | ||
} | ||
|
||
[data-group~='sliders'] { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,40 +1,84 @@ | ||
import { | ||
clone, | ||
display, | ||
inGamut, | ||
type PlainColorObject, | ||
serialize, | ||
set, | ||
steps, | ||
to, | ||
} from 'colorjs.io/fn'; | ||
|
||
import { type ColorFormatId, FORMATS } from '$lib/constants'; | ||
import { | ||
type ColorFormatId, | ||
type ColorGamutId, | ||
FORMATS, | ||
GAMUT_IDS, | ||
} from '$lib/constants'; | ||
|
||
export const getSpaceFromFormatId = (formatId: ColorFormatId) => | ||
formatId === 'hex' ? 'srgb' : formatId; | ||
|
||
export const sliderGradient = ( | ||
color: PlainColorObject, | ||
channel: string, | ||
range: [number, number], | ||
) => { | ||
export const sliderGradient = ({ | ||
color, | ||
channel, | ||
range, | ||
gamut, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
}: { | ||
color: PlainColorObject; | ||
channel: string; | ||
range: [number, number]; | ||
gamut: ColorGamutId; | ||
}) => { | ||
const start = clone(color); | ||
const end = clone(color); | ||
if (channel === 'alpha') { | ||
start.alpha = range[0]; | ||
end.alpha = range[1]; | ||
start.alpha = 0; | ||
end.alpha = 1; | ||
} else { | ||
set(start, channel, range[0]); | ||
start.alpha = 1; | ||
set(end, channel, range[1]); | ||
end.alpha = 1; | ||
Comment on lines
+40
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@SondraE Were you trying it on this deploy preview (https://deploy-preview-226--oddcontrast.netlify.app/), or on the current production app (https://www.oddcontrast.com/)? The question is about a change in this PR. Currently in production, when you lower the alpha value (e.g. this example), the background color of the other sliders also adjust to factor in your alpha value. The change @jamesnw is proposing is to have the other slider background colors always show with full alpha (e.g. this example), which makes their values clearer but also not representative of the current color. Current production: Proposal in this PR: |
||
} | ||
|
||
const gradientSteps = steps(start, end, { | ||
steps: 10, | ||
space: color.space, | ||
hue: 'raw', | ||
// Smaller values will take longer, larger will be less precise and | ||
// produce fuzzy edges. This magic number seems to balance that. | ||
maxDeltaE: 10, | ||
jgerigmeyer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
let wasInGamut = true; | ||
const inGamutSteps: string[] = []; | ||
const stepWidth = 100 / (gradientSteps.length - 1); | ||
|
||
if (channel === 'alpha' || gamut === null) { | ||
return gradientSteps.map((c) => display(c)).join(', '); | ||
} | ||
|
||
// Create a linear gradient string, mapping gradientSteps to 0%-100% by | ||
// multiplying its index with `stepWidth`. | ||
gradientSteps.forEach((step, index) => { | ||
if (inGamut(step, gamut)) { | ||
if (wasInGamut === false) { | ||
// Coming back into gamut. Add a transparent gradient step for a crisp | ||
// edge. | ||
inGamutSteps.push(`transparent ${stepWidth * index}%`); | ||
} | ||
wasInGamut = true; | ||
inGamutSteps.push(`${display(step)} ${stepWidth * index}%`); | ||
} else if (wasInGamut === true) { | ||
// Leaving gamut. Add a transparent gradient step at the same percent as | ||
// the previous in gamut step for a crisp edge. | ||
inGamutSteps.push(`transparent ${stepWidth * (index - 1)}%`); | ||
|
||
return gradientSteps.map((c) => display(c)).join(', '); | ||
wasInGamut = false; | ||
} | ||
}); | ||
|
||
return inGamutSteps.join(', '); | ||
}; | ||
|
||
function decodeColor(colorHash: string, format: ColorFormatId) { | ||
|
@@ -63,11 +107,13 @@ export const hashToStoreValues = ( | |
bg: PlainColorObject; | ||
fg: PlainColorObject; | ||
format: ColorFormatId; | ||
gamut: ColorGamutId; | ||
} | void => { | ||
if (hash === '') return; | ||
hash = decodeURIComponent(hash); | ||
|
||
const [formatValue, bgValue, fgValue] = hash.split('__') as [ | ||
const [formatValue, bgValue, fgValue, gamutValue] = hash.split('__') as [ | ||
string, | ||
string, | ||
string, | ||
string, | ||
|
@@ -77,20 +123,26 @@ export const hashToStoreValues = ( | |
if (!FORMATS.includes(formatValue as ColorFormatId)) return; | ||
const format = formatValue as ColorFormatId; | ||
|
||
const gamut = GAMUT_IDS.includes(gamutValue as ColorGamutId) | ||
? (gamutValue as ColorGamutId) | ||
: null; | ||
|
||
const bg = decodeColor(bgValue, format); | ||
if (!bg) return; | ||
const fg = decodeColor(fgValue, format); | ||
if (!fg) return; | ||
|
||
return { bg, fg, format }; | ||
return { bg, fg, format, gamut }; | ||
}; | ||
|
||
export const storeValuesToHash = ( | ||
bg: PlainColorObject, | ||
fg: PlainColorObject, | ||
format: ColorFormatId, | ||
gamut: ColorGamutId, | ||
) => { | ||
const bgParam = encodeColor(bg, format); | ||
const fgParam = encodeColor(fg, format); | ||
return encodeURIComponent(`${format}__${bgParam}__${fgParam}`); | ||
const gamutParam = gamut ? `__${gamut}` : ''; | ||
return encodeURIComponent(`${format}__${bgParam}__${fgParam}${gamutParam}`); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { fireEvent, render } from '@testing-library/svelte'; | ||
import { get } from 'svelte/store'; | ||
|
||
import Gamut from '$lib/components/GamutSelect.svelte'; | ||
import { gamut, INITIAL_VALUES, reset } from '$lib/stores'; | ||
|
||
describe('Space', () => { | ||
afterEach(() => { | ||
reset(); | ||
}); | ||
|
||
it('renders editable gamut select', async () => { | ||
const { getByLabelText } = render(Gamut); | ||
|
||
expect(get(gamut)).toBe(INITIAL_VALUES.gamut); | ||
|
||
const select = getByLabelText('Gamut'); | ||
await fireEvent.change(select, { target: { value: 'rec2020' } }); | ||
|
||
expect(get(gamut)).toBe('rec2020'); | ||
}); | ||
}); |
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?