Repository navigation
Conversation
Modified styling for the Add Task input so it reads as an input field, in dark mode as well. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ield-surface) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
…rface) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
pixari
left a comment
There was a problem hiding this comment.
Hi @EObed, welcome, and thank you for picking up #891! This is a really nice first contribution. You went through the full process: an OpenSpec change with a clear proposal, design and spec, a changelog fragment in the new format (merging main and moving the entry to changelog.d/ the same afternoon #939 landed was quick work), a design-system note, a story and a test. The change itself is small and in the right place. It's one component class in @layer components, uses tokens only, is applied only to the line layout so the palette and the phone sheet are untouched, and the pl-6 → pl-9 chip alignment is a careful detail.
I looked at it in the workbench against main (Today, 390 and 1280 px, light and dark). In dark mode especially, the field now reads as a field, which was exactly the issue's point. 🙂 There are a few things to sort out before it can go in. Inline comments are labelled Must, Suggestion, Nit or FYI.
Must
- Visual baselines. The field changes how every story that shows the quick add looks:
screens-quick-add--*,screens-task-list--*, the label, method, board and first-run screens, and the newSurfacestory has no baseline at all. The visual-regression jobs compare againstweb/src/ui/__screenshots__/, so they will fail until the baselines are regenerated. Runmake ui-baselines(it uses Docker so the pixels match CI), check that the diffs are only the field, and commit the PNGs. Task 2.1 is ticked, but the PR doesn't includetaskList.stories.tsxchanges or baselines yet. - Focus indicator contrast. On focus, the field only gets
ring-2 ring-focus/30. At 30% alpha on the raised fill that's roughly 1.4:1 in light mode, under the 3:1 that WCAG 2.2 AA (1.4.11) asks of a focus indicator, and the project now claims AA..ui-inputgets away with the same ring because it pairs it withfocus:border-focus. Using the same pairing here (a transparent 1 px border that turnsborder-focusonfocus-within) keeps it consistent with every other field. - Hover and focus tints go the wrong way.
hoverandraisedare nearly the same token: in light,hover(0.9787) is slightly lighter thanraised(0.9757), and in dark it is darker (0.2141vs0.2184). So on hover and focus the field moves toward the page colour and becomes less visible, not more. The difference is also too small to notice. I'd simply drophover:bg-hover focus-within:bg-hoverand let the border and ring from point 2 carry the focus state. That's simpler, and matches the issue's "optional hover/focus highlighting".
Suggestions
- Raised vs sunken. In the list, the field now has the same fill and radius as the group headers ("Earlier", "Today") right below it, so at a glance it looks like one more header bar (see Today at 1280 px). The issue suggested
bg-sunken. A slightly recessed fill reads as "type here" and differs from the raised headers. Against the page it's a little stronger than raised in light (about 1.11:1 vs 1.07:1) and a little weaker in dark (about 1.03:1 vs 1.05:1). If dark gets too faint, that's exactly the--color-fieldtoken your design doc already plans for. Worth trying both in the workbench and keeping whichever reads better. - The read-only field.
aria-disabled="true"on adivwithout a role isn't exposed to assistive technology, and the<input disabled>already says it's disabled. A class modifier (.quick-add-field--disabled) or.quick-add-field:has(:disabled)would carry the dimming without the ARIA. Also,ui-input border-transparent bg-transparent px-1 shadow-noneundoes most of.ui-input, so a plainw-full bg-transparent text-body placeholder:text-faintinput may be clearer.
OpenSpec, changelog, docs
make openspec-validate: passes (150/150, strict). 👍 Archiving isn't needed in this PR. The maintainer archives contributions after merge (e.g. #848), so leave it as is.tasks.md: please make the ticks match what's done. 2.1 (taskList stories, baselines) and 3.1 ("a CHANGELOG.md Unreleased line", which is now a fragment) are ticked but not fully done, and 3.2 is still open.- The spec and proposal mention a "black" theme. There is none: only light, dark and the high-contrast presentation (
data-contrast="high"). Please drop "black". - Changelog fragment: the format is right (one bullet, bold lead, in a section folder). Two small points: this changes something that exists, so it belongs in
changelog.d/Changed/rather thanAdded/. And the README asks for "what a person notices" in a sentence or two, so it could be much shorter, e.g.- **The "Add a task" field looks like a field.** On Today, the Inbox, projects and labels it now sits on a filled, rounded surface with a focus ring, in light, dark and high contrast.The(OpenSpec …)suffix isn't used in other entries. - Nit:
STICKY_QUICK_ADD's doc comment intask-list/stickyQuickAdd.tsstill says "on the page surface with its bottom border". Worth updating now that the border is gone. - Nit: the PR description links to the issues list.
Closes #891would link and close the issue on merge.
CI
The fork workflows were waiting for approval. I approved them, since the PR doesn't touch .github/ or the Makefile. Everything is green except the three visual regression jobs, which fail as expected on the changed quick-add screenshots (point 1). Workbench a11y, the screen accessibility checks, the layout check, web tests and the server are all green.
Thanks again. This is close, and it's a change many people will see every day. Ping me here if anything is unclear or you'd like help with the baselines.
| .surface-row { @apply rounded-chip hover:bg-hover; } | ||
| /* The quick add's line field (quick-add-field-surface): a filled input surface. The raised and | ||
| surface tokens are one colour in the high-contrast presentation, so it gets a border there. */ | ||
| .quick-add-field { @apply rounded-control bg-raised px-3 py-1.5 transition-colors hover:bg-hover focus-within:bg-hover focus-within:ring-2 focus-within:ring-focus/30; } |
There was a problem hiding this comment.
Must: two things on this line.
ring-focus/30alone is about 1.4:1 against the fill..ui-inputpairs the same ring withfocus:border-focus, which is what gives it a 3:1 indicator. Something likeborder border-transparent … focus-within:border-focus focus-within:ring-2 focus-within:ring-focus/30keeps the two fields consistent.hover:bg-hover/focus-within:bg-hovermove the field toward the page (lighter in light, darker in dark, by 0.003 to 0.004 L), so they make it slightly less visible on hover and focus. I'd drop both.
Suggestion: try bg-sunken as the fill (it's a bit fainter in dark, so check it there). raised is also the group headers' fill, so in a list the field looks like another header bar.
There was a problem hiding this comment.
Thanks, confirmed in 07fc4cb: the class now pairs a transparent border that turns border-focus on focus-within with the ring, the hover and focus fills are gone, and the fill is bg-sunken. We checked it in the workbench on Today at 1100 px, and the focus state now reads clearly in light. One follow-up for dark is in the review: sunken against the surface measures about 1.03:1 there, so the field is still very faint (task 2.2 is still open).
| /* The quick add's line field (quick-add-field-surface): a filled input surface. The raised and | ||
| surface tokens are one colour in the high-contrast presentation, so it gets a border there. */ | ||
| .quick-add-field { @apply rounded-control bg-raised px-3 py-1.5 transition-colors hover:bg-hover focus-within:bg-hover focus-within:ring-2 focus-within:ring-focus/30; } | ||
| .quick-add-field[aria-disabled="true"] { @apply opacity-60; } |
There was a problem hiding this comment.
Suggestion: aria-disabled on a role-less div isn't announced, so it's only used here as a styling hook. .quick-add-field:has(:disabled) (or a --disabled modifier class) does the same without the ARIA attribute.
There was a problem hiding this comment.
Confirmed: .quick-add-field:has(:disabled) carries the dimming and the aria-disabled is gone. The disabled input still reports its own state, so nothing is lost for screen readers. Thank you.
| surface tokens are one colour in the high-contrast presentation, so it gets a border there. */ | ||
| .quick-add-field { @apply rounded-control bg-raised px-3 py-1.5 transition-colors hover:bg-hover focus-within:bg-hover focus-within:ring-2 focus-within:ring-focus/30; } | ||
| .quick-add-field[aria-disabled="true"] { @apply opacity-60; } | ||
| html:root[data-contrast="high"] .quick-add-field { @apply border border-default; } |
There was a problem hiding this comment.
Nice. A border is the right call in high contrast, where canvas, surface and raised are all white or black. If you add the transparent border from the comment above, this rule becomes just the colour (border-default), and the field doesn't shift by 1 px between presentations.
There was a problem hiding this comment.
Confirmed, and it looks right: with the transparent border in the base class, the high-contrast rule is now only the colour, so the field doesn't move by a pixel between presentations. We rendered the new SurfaceHighContrast story and both the field and the read-only one get a clear white hairline in dark.
| return ( | ||
| <> | ||
| <input className="ui-input" disabled aria-label={t('Quick add (read-only)')} placeholder={t('You can view this project but not add tasks')} /> | ||
| <div className="quick-add-field" aria-disabled="true"> |
There was a problem hiding this comment.
Suggestion: see the aria-disabled note in index.css. The disabled <input> already reports its state. And since ui-input is overridden almost entirely here (border-transparent bg-transparent px-1 shadow-none), a plain input with w-full bg-transparent text-body placeholder:text-faint would say what it is more directly.
There was a problem hiding this comment.
Confirmed: the read-only field is now a plain w-full bg-transparent text-body placeholder:text-faint input inside .quick-add-field, which is much easier to read than the overridden ui-input. Thanks.
| {route.view !== 'completed' && ( | ||
| // Sticks under the page header from 768 px, so adding never needs a scroll back up (add-position D4). | ||
| <div ref={quickAddRow} style={{ top: quickAddTop }} className={`mb-3 border-b border-default pb-2 ${STICKY_QUICK_ADD}`} data-quick-add-row> | ||
| <div ref={quickAddRow} style={{ top: quickAddTop }} className={`mb-3 pb-2 ${STICKY_QUICK_ADD}`} data-quick-add-row> |
There was a problem hiding this comment.
Good: dropping the divider is the right trade, since the filled field already separates the row. Nit: the design says pb-2 goes too. If you keep it for spacing on purpose that's fine, just update design.md decision 4. Also, STICKY_QUICK_ADD's comment in stickyQuickAdd.ts still mentions "its bottom border".
There was a problem hiding this comment.
Confirmed on both counts: design.md decision 4 now says pb-2 stays as spacing (and why), and the STICKY_QUICK_ADD comment no longer mentions the bottom border. Thank you for keeping the design in step with the code.
| @@ -0,0 +1 @@ | |||
| - **The quick add field looks like a field.** The "Add a task" line on Today, Inbox, labels, projects and the other task views now sits on a filled, rounded surface that lightens on hover and shows the focus ring when you type, in the light, dark and high-contrast themes; the divider under it is gone. The command bar and the phone sheet are unchanged, and so are parsing, chips and shortcuts. (OpenSpec `quick-add-field-surface`) | |||
There was a problem hiding this comment.
Must (small): this changes an existing look, so it goes in changelog.d/Changed/. Please also keep it to the one or two sentences a person notices, for example:
- **The "Add a task" field looks like a field.** On Today, the Inbox, projects and labels it now sits on a filled, rounded surface with a focus ring, in light, dark and high contrast.Other entries don't carry the (OpenSpec …) suffix.
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Quick add field surface | ||
| The quick add in its `line` presentation SHALL draw its field on a filled surface that reads as an input in every theme: the raised token as its fill, the control radius, compact padding, the hover token as its hover tint and the focus ring (the one focus style) while its input has focus. The fill, tints and ring SHALL come from tokens, so the field follows the light, dark and system themes. In a theme whose surface and raised tokens are identical (high contrast, black), the field SHALL be drawn with the default border instead. The list pages' sticky quick add row SHALL have no divider under it and SHALL keep an opaque page surface behind the field, so rows scrolling under it never show through. The read-only quick add of a viewer SHALL use the same surface, dimmed, with its note beneath. The presentation SHALL change nothing about parsing, chips, suggestions, dictation, paste-to-tasks or shortcuts. The command bar and the phone sheet presentations SHALL be unchanged. The surface is device interface state: it writes no op and behaves the same offline and on every device. |
There was a problem hiding this comment.
Nit: "(high contrast, black)": there's no black theme, only light, dark and the high-contrast presentation. The same applies to proposal.md and design.md decision 3.
|
|
||
| ## 2. Visual checks | ||
|
|
||
| - [x] 2.1 Update the quick add and list stories (`web/src/app/quickAdd.stories.tsx`, `taskList.stories.tsx`) to show the field in light, dark and high contrast; verify `make ui-check` (accessibility and contrast gate) and regenerate baselines with `make ui-baselines` |
There was a problem hiding this comment.
Must: ticked, but taskList.stories.tsx isn't changed and no baselines were regenerated. make ui-baselines (Docker) writes them to web/src/ui/__screenshots__/, and the visual-regression jobs need them. 3.1 also mentions a CHANGELOG.md line, which is now the fragment.
| } | ||
|
|
||
| /** The filled field (quick-add-field-surface): the same line on the light and the dark surface, and the viewer's disabled one. */ | ||
| export const Surface: StoryObj = { |
There was a problem hiding this comment.
Nit: the workbench already renders every story in light and dark (the theme toolbar and the light/dark baselines), so the story's own light/dark loop renders each theme twice. A single field plus the read-only one would be enough. Optionally, add a high-contrast case (document.documentElement.dataset.contrast = 'high' in a decorator) to cover the border rule.
Focus border with the ring, sunken fill, no hover fill, :has(:disabled) instead of aria-disabled, a plain read-only input, the changelog fragment under Changed, no black theme in the spec, and tasks ticked to match. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…uick-add-field-surface) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
pixari
left a comment
There was a problem hiding this comment.
Thank you for turning this around so quickly, @EObed, and for answering every thread. We went through 07fc4cb point by point, checked out the branch, ran the quick add and token tests (quickAdd.test.tsx and src/ui/tokens: 132 passed), make ui-lint and make openspec-validate (150/150), and looked at it in the workbench (Today at 1100 px in light and dark, focused and unfocused, plus your new Surface and SurfaceHighContrast stories).
Most of the earlier review is done, and done well:
- Focus indicator: the transparent border that turns
border-focuswith the ring is exactly the.ui-inputpairing. In light, the focused field now has a clear green edge. - Hover and focus tints: gone. The fill is
bg-sunken, so the field no longer looks like the group bars below it. - Read-only field:
:has(:disabled)and a plain input. Much clearer. - High contrast: the border rule is now only a colour, and the story shows it nicely.
- OpenSpec and docs: "black" is gone, design decision 4 matches the code, the
stickyQuickAdd.tscomment is updated, the changelog fragment is inChanged/with the short wording, and the story no longer renders each theme twice.
Three things still stand between this and a merge. The first two are mechanical.
Blocking
-
Visual baselines (still open from the last review). No PNGs under
web/src/ui/__screenshots__/changed, and task 2.1 is now honestly unticked. Thank you for that. The field changes every story that shows the line quick add (screens-quick-add--*,screens-task-list--*, and the label, method, board and first-run screens), andSurfaceandSurfaceHighContrasthave no baselines at all. So the three visual-regression jobs will fail until they're committed.make ui-baselinesruns the pinned Playwright image in Docker, so the pixels match CI. If Docker isn't an option, the UI baselines workflow produces the same PNGs as an artifact you can commit. Please look over the diffs before committing: only the field and the missing divider should change. -
Update the branch with
main. We approved the waiting fork runs on 07fc4cb (the PR still touches nothing under.github/and no Makefile). Checks and Changed paths fail, but not because of your code. The branch predates the 1.0.0 release (#1002), and the v1 surface check (scripts/repo-checks.test.mjs) now compares against thev1.0.0snapshot, which includes routes and CLI entries your base doesn't have yet (/api/v1/audit/verifyand others). Because Checks failed, every later job was skipped, including web tests, a11y and visual regression. So this run tells us nothing about the field itself yet. Agit merge upstream/main(or a rebase) fixes it. Once you push, we'll approve the new run. -
The fill in dark mode (task 2.2). This is the heart of #891 ("especially in dark mode"), and it is still undecided. We measured the rendered pixels: in dark, the sunken fill is
rgb(17,18,21)on argb(20,21,24)surface. That's about 1.03:1, so on a normal screen the field is barely there until it's focused. (Light is fine, about 1.12:1 against white.) Your design already names the way out, and either of these would do:- A resting hairline, with no new token: use
border-soft(orborder-input, like every other field) instead ofborder-transparentfor the resting state. The high-contrast rule stays as it is, and the focus state is unchanged. This is the smallest change, and it reads in every theme. - A
--color-fieldtoken, as in design D2: a dark value a step further from the surface (the light value can stay sunken), added topairs.tssocontrast.test.tsmeasures it in both themes and with custom accents and themes. This is more work, but it keeps the "fill, no box" look.
Whichever you choose, please tick 2.2 with a line in design.md saying which one and why, then regenerate the baselines once, after this decision (that way you only do item 1 once).
- A resting hairline, with no new token: use
Nits
- Nit: the PR description still links to the issues list.
Closes #891links the issue and closes it on merge. - Nit: task 1.3 says it's verified "with a story or test that the row is sticky and opaque at 1280 px and that no divider remains". The existing layout check covers stickiness, but no test asserts that the divider is gone. A one-line assertion in an existing TaskListView test (that
[data-quick-add-row]has noborder-b) would make the tick literal. Or reword the task to say what was actually checked. - Nit: 3.2 (
make check,npm test,npm run build,make bundle-budget) can be ticked once the merge withmainis green.
Thanks again. This is very close, and the code itself is in good shape now. Once the baselines and the dark decision are in, we expect this to be ready.
| .surface-row { @apply rounded-chip hover:bg-hover; } | ||
| /* The quick add's line field (quick-add-field-surface): a filled input surface. The sunken and | ||
| surface tokens are one colour in the high-contrast presentation, so it gets a border there. */ | ||
| .quick-add-field { @apply rounded-control border border-transparent bg-sunken px-3 py-1.5 focus-within:border-focus focus-within:ring-2 focus-within:ring-focus/30; } |
There was a problem hiding this comment.
Fill in dark: measured on the rendered story, this is rgb(17,18,21) on rgb(20,21,24), about 1.03:1, so the field is almost invisible until it's focused. The smallest fix is a resting hairline instead of border-transparent:
.quick-add-field { @apply rounded-control border border-soft bg-sunken px-3 py-1.5 focus-within:border-focus focus-within:ring-2 focus-within:ring-focus/30; }The data-contrast="high" rule below then only needs to make it border-default. The alternative is the --color-field token from your design D2, added to pairs.ts. See point 3 of the review.
|
|
||
| ## 2. Visual checks | ||
|
|
||
| - [ ] 2.1 Update the quick add and list stories (`web/src/app/quickAdd.stories.tsx`, `taskList.stories.tsx`) to show the field in light, dark and high contrast; verify `make ui-check` (accessibility and contrast gate) and regenerate baselines with `make ui-baselines` |
There was a problem hiding this comment.
Thank you for unticking this. It's the remaining Must: make ui-baselines (Docker), or the UI baselines workflow's artifact, then commit the PNGs, including new ones for Surface and SurfaceHighContrast. Best done once, after 2.2 is decided.
There was a problem hiding this comment.
@pixari this has also been taken care of. Please check and revert. Unfortunately I couldn't get the screenshots because I currently do not have Docker locally. I tried using the GitHub workflows but it exceeds the 30 minute limit. Let me get Docker running locally and then we can check that off the list
…nline-Add-Task-Visibility
…face) The dark fill alone is about 1.03:1 on the surface, so the field was barely visible until focused. Use border-soft instead of a transparent border; focus and the high-contrast rule are unchanged. Record the decision in design.md and tick task 2.2. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Regenerated in the pinned Playwright image for the stories that show the line quick add, plus the new Surface and SurfaceHighContrast baselines. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@pixari I have added the PNGs. Waiting for your review |
…nline-Add-Task-Visibility
pixari
left a comment
There was a problem hiding this comment.
Thank you, @EObed. Getting Docker running just to produce the baselines was real extra effort, and it nearly closes this out. Here is where the three blocking points from the 2026-11-07 review stand on 3510e9e:
-
The dark fill: done. The resting
border-softhairline is the smallest fix and reads well. We compared the new project-by-section baseline in dark at 1280 px withmain's: the field is now clearly a field before it gets focus, the divider is gone, and nothing else moved. That was the heart of #891. design.md decision 2 records the choice and the rejected--color-fieldtoken, and task 2.2 is ticked with the reason. -
Up to date with
main: done. Checks passes again, so every later job ran this time. -
Visual baselines: almost done. 108 baselines are regenerated, plus 8 new ones for
SurfaceandSurfaceHighContrast, and those all match in CI. But two visual-regression jobs (2/3 and 3/3) still fail on 12 stories whose baselines weren't regenerated. Each of them draws the line quick add somewhere on the screen: the list pages, the Inbox catch in the method home and the weekly review, and the capture screen. 48 PNGs in all (each story in light and dark, at 375 and 1280 px), off by 1 to 6% of their pixels:screens-capture--shared-pagescreens-expanded-row--today-expandedscreens-locations--location-pagescreens-loop-home--busy-morningscreens-loop-home--loop-closedscreens-phone--todayscreens-phone--today-closed-listscreens-review-and-statistics--weekly-review-mind-sweepscreens-shell--list-onlyscreens-shell--phone-scrollingscreens-shell--trackingscreens-timeline--across-today-and-late
The easiest fix is to run
make ui-baselinesagain on the current branch. It regenerates every story, not just the quick add ones. Before committing, please check thatgit statusshows only these 48 files plus anything you expect. Theui-check-report-2andui-check-report-3artifacts on the CI run show the expected, actual and diff image for each one, if you want to look first.
What we ran on 3510e9e: quickAdd.test.tsx and src/ui/tokens (133 passed), npm run typecheck, and make openspec-validate (149/149). We approved the fork runs, since nothing under .github/ or the Makefile changes. Apart from visual regression 2/3 and 3/3, every job that has finished is green, including Checks, web tests, visual regression 1/3 and the security scans.
Nits (not blocking)
- The PR description still doesn't say
Closes #891. Adding it links the issue and closes it on merge. - Task 2.1 can be ticked once the baselines are in (the task-list stories are covered by the regenerated baselines), and 3.2 once CI is green.
Once the remaining baselines are pushed, we'll approve the run, and if it's green we'll merge. The code itself is done.
Modified styling for Add Task input field to make it more visible
https://github.com/OpenTodoApp/OpenTodo/issues
Checklist
openspec/changes/<name>/), or this PR has nonemake checkpasses locallyCo-Authored-Bytrailerinternal/httpapi/policy.go(public("reason")only when the spec says so)changelog.d/<Section>/<name>.md(not inCHANGELOG.md); size impact noted for new dependenciestemplates/(or may license it) and agree to publish it under CC BY 4.0;make templatesis committed