-
Notifications
You must be signed in to change notification settings - Fork 7
Self-hosted: give the accent a colour control and tell the owner when it is wrong #1364
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
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -3,9 +3,13 @@ import { join } from 'node:path'; | |
| import ts from 'typescript'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { HIVE_LAYER_CONFIG_DEFAULTS } from '@/core/hive-layer'; | ||
| import { parseHexColor } from '@/core/theme-appearance'; | ||
| import type { ConfigField } from './config-fields'; | ||
| import { configFieldsMap } from './config-fields'; | ||
| import { | ||
| COLOR_UNSET_HINT, | ||
| colorInputMessage, | ||
| colorPickerValue, | ||
| displayedBooleanValue, | ||
| displayedSelectValue, | ||
| displayedStringValue, | ||
|
|
@@ -396,3 +400,142 @@ describe('a select whose resolver normalizes', () => { | |
| expect(displayedSelectValue(strict, 'Standard')).toBe('off'); | ||
| }); | ||
| }); | ||
|
|
||
| /** | ||
| * A bare text field accepted `banana`, saved it, and the site kept the template | ||
| * colour with nothing said. The save succeeded, so the natural reading was that | ||
| * the feature was broken. | ||
| * | ||
| * Everything here is checked against `parseHexColor`, the function the | ||
| * appearance engine itself uses, rather than against a second regex. A panel | ||
| * with its own opinion about the same string is how the panel and the site come | ||
| * to disagree. | ||
| */ | ||
| describe('color input', () => { | ||
| it('says nothing about a value the engine accepts', () => { | ||
| for (const accepted of ['#0969da', '#abc', '#ABC', ' #0969da ']) { | ||
| expect(colorInputMessage(accepted), accepted).toBeNull(); | ||
| } | ||
| }); | ||
|
|
||
| it('warns about anything the engine will not apply', () => { | ||
| for (const rejected of ['banana', '#12', '#0969d', 'rgb(1,2,3)', '0969da']) { | ||
| const note = colorInputMessage(rejected); | ||
| expect(note?.invalid, rejected).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Alpha is refused by the engine on purpose: a translucent fill is a | ||
| * readability hole no contrast correction can close. The panel has to refuse | ||
| * it too, or it silently accepts a colour the site drops. | ||
| */ | ||
| it('warns about an alpha hex, which the engine refuses', () => { | ||
| expect(colorInputMessage('#0969daff')?.invalid).toBe(true); | ||
| }); | ||
|
|
||
| /** Empty is a real state, not an error: it means the template's own colour. */ | ||
| it('explains empty rather than flagging it', () => { | ||
| const note = colorInputMessage(''); | ||
| expect(note?.invalid).toBe(false); | ||
| expect(note?.message).toBe(COLOR_UNSET_HINT); | ||
| expect(colorInputMessage(' ')?.invalid).toBe(false); | ||
| }); | ||
|
|
||
| /** | ||
| * The warning has to say what happens, because what happens is the confusing | ||
| * part: the save succeeds and the site does not change. | ||
| */ | ||
| it('says what the site will do with a value it refuses', () => { | ||
| expect(colorInputMessage('banana')?.message).toMatch(/template/i); | ||
| }); | ||
|
|
||
| /** | ||
| * Agreement with the engine, asserted as a property over both rather than as | ||
| * two lists someone kept in step. | ||
| */ | ||
| it.each([ | ||
| '#0969da', | ||
| '#abc', | ||
| 'banana', | ||
| '', | ||
| '#0969daff', | ||
| 'rgb(0,0,0)', | ||
| '#GGGGGG', | ||
| ])('flags %s exactly when the engine refuses it', (text) => { | ||
| const engineAccepts = parseHexColor(text) !== null; | ||
| const panelFlags = colorInputMessage(text)?.invalid === true; | ||
| // Empty is the one value that is neither accepted nor an error. | ||
| if (text.trim() === '') { | ||
| expect(panelFlags).toBe(false); | ||
| return; | ||
| } | ||
| expect(panelFlags).toBe(!engineAccepts); | ||
| }); | ||
| }); | ||
|
|
||
| describe('color swatch value', () => { | ||
| /** `<input type="color">` only accepts `#rrggbb`, so `#abc` has to expand. */ | ||
| it('expands a short hex the native control cannot take', () => { | ||
| expect(colorPickerValue('#abc')).toBe('#aabbcc'); | ||
| }); | ||
|
|
||
| it('passes a full hex through, lower-cased and trimmed', () => { | ||
| expect(colorPickerValue(' #0969DA ')).toBe('#0969da'); | ||
| }); | ||
|
|
||
| /** | ||
| * The swatch has no empty state, so it needs something concrete. Displaying | ||
| * it must not write it: a value reaches the document only through onChange, | ||
| * so an owner who opens the panel and saves without touching the swatch | ||
| * stores nothing. | ||
| */ | ||
| it('falls back for unset and unparseable values without inventing one', () => { | ||
| expect(colorPickerValue('')).toBe('#888888'); | ||
| expect(colorPickerValue('banana')).toBe('#888888'); | ||
| expect(colorPickerValue('', '#123456')).toBe('#123456'); | ||
| }); | ||
| }); | ||
|
|
||
| /** | ||
| * That the renderer actually uses the validation above. | ||
| * | ||
| * Every test in this file passed with the entire `case 'color'` block deleted | ||
| * from `config-editor.tsx`, which is the whole point of the change: the helpers | ||
| * being correct means nothing if the panel does not call them. Nothing in a | ||
| * `.tsx` is renderable under this runner, so the call is what can be asserted, | ||
| * and here the call IS the mechanism. | ||
| */ | ||
| describe('the editor renders colour fields with the shared validation', () => { | ||
| const EDITOR = join(__dirname, 'components', 'config-editor.tsx'); | ||
|
|
||
| function calledFunctions(source: string): Set<string> { | ||
| const file = ts.createSourceFile( | ||
| 'config-editor.tsx', | ||
| source, | ||
| ts.ScriptTarget.Latest, | ||
| true, | ||
| ts.ScriptKind.TSX, | ||
| ); | ||
| const called = new Set<string>(); | ||
| const visit = (node: ts.Node): void => { | ||
| if (ts.isCallExpression(node) && ts.isIdentifier(node.expression)) { | ||
| called.add(node.expression.text); | ||
| } | ||
| ts.forEachChild(node, visit); | ||
| }; | ||
| visit(file); | ||
| return called; | ||
| } | ||
|
|
||
| it('handles the color type and uses both helpers', () => { | ||
| const source = readFileSync(EDITOR, 'utf8'); | ||
| const called = calledFunctions(source); | ||
|
|
||
| expect(source).toContain("case 'color'"); | ||
| // The message, or an invalid value is stored with nothing said. | ||
| expect(called).toContain('colorInputMessage'); | ||
| // The swatch value, or the native control gets a string it cannot take. | ||
| expect(called).toContain('colorPickerValue'); | ||
| }); | ||
| }); | ||
|
Comment on lines
+500
to
+541
Contributor
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. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Files matching target:"
fd 'field-display\.test\.ts$|config-editor\.tsx$|components' apps/self-hosted/src/features/floating-menu || true
target_test="apps/self-hosted/src/features/floating-menu/field-display.test.ts"
target_editor="apps/self-hosted/src/features/floating-menu/components/config-editor.tsx"
if [ -f "$target_test" ]; then
echo "--- test file size ---"
wc -l "$target_test"
echo "--- lines 460-560 ---"
sed -n '460,560p' "$target_test" | nl -ba -v460
else
echo "test file missing"
fi
if [ -f "$target_editor" ]; then
echo "--- editor file size ---"
wc -l "$target_editor"
echo "--- outline ---"
ast-grep outline "$target_editor" || true
echo "--- color-related snippets ---"
rg -n "color|colour|colorInputMessage|colorPickerValue|Clear|onUpdate" "$target_editor" -C 3
fi
echo "--- package/test framework indicators ---"
fd -a 'package\.json$' . | xargs -r -I{} sh -c 'echo "--- {}"; jq -r ".scripts // {}, .devDependencies // {}, .dependencies // {} | {vitest: .vitest, "`@testing-library/react`": .["`@testing-library/react`"], react: .react, vitest?: .[]? // empty}" {} 2>/dev/null || true'Repository: ecency/vision-web Length of output: 529 🏁 Script executed: #!/bin/bash
set -euo pipefail
target_test="apps/self-hosted/src/features/floating-menu/field-display.test.ts"
target_editor="apps/self-hosted/src/features/floating-menu/components/config-editor.tsx"
echo "--- target editor size ---"
wc -l "$target_editor"
echo "--- target test size/lines 440-542 ---"
python3 - <<'PY'
from pathlib import Path
p = Path("apps/self-hosted/src/features/floating-menu/field-display.test.ts")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 440 <= i <= 542:
print(f"{i:4}: {line}")
PY
echo "--- outline ---"
ast-grep outline "$target_editor" || true
echo "--- all color-related snippets in config-editor.tsx ---"
rg -n "color|colour|colorInputMessage|colorPickerValue|Clear|onUpdate" "$target_editor" -C 3 || true
echo "--- relevant test helpers ---"
python3 - <<'PY'
from pathlib import Path
text = Path("apps/self-hosted/src/features/floating-menu/field-display.test.ts").read_text()
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 80 or 400 <= i <= 555:
print(f"{i:4}: {line}")
PYRepository: ecency/vision-web Length of output: 17910 Replace the source-inspection test with a rendered editor test. This test only reads 🤖 Prompt for AI AgentsSource: Coding guidelines
Member
Author
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. Not taking this one here: the runner is environment 'node' with include limited to *.test.ts, and @testing-library is not a dependency anywhere in this app, so a rendered-editor test is an infrastructure decision rather than a fix for this PR. The AST guard stays as the stopgap; it provably catches the deleted-case mutation. |
||
Uh oh!
There was an error while loading. Please reload this page.