Fix accessibility and UX defects - #1218
Closed
slick-daddy wants to merge 6 commits into
Closed
Conversation
Calling navigate() in the component body triggers React updates during render and can loop while auth state resolves. Return a declarative <Navigate to=/ replace /> after all hooks instead. Found by /impeccable audit (P1).
- Selected value and typed input were forced to black on the dark $secondary control (~2.3:1); they now inherit the control's $text-color (~9.9:1). - Multi-value chips paired near-white text with a near-white chip; use $dark-text for ~9.6:1. - StudioSelect parent label used rgba(black, 0.5) inside the value container, rendering near-invisible; drop the override so it uses the legible $text-muted applied elsewhere. Brings all select surfaces to WCAG AA text contrast. Found by /impeccable audit (P1).
Deleted images were rendered as srcless <img> tags, which browsers draw as zero-width boxes, leaving blank gaps where removed images should appear in edit diffs. Add a shared DeletedImage fragment (muted tile with an X icon and Deleted label, matching the existing empty-thumbnail language) and use it in ImageChangeRow and AmendableImageChangeRow. Found by /impeccable audit (P1).
The hard min-width on body, carried over from the Bootstrap 5 migration, locked every viewport narrower than 1210px into permanent horizontal scrolling of the entire app. The navbar already wraps (react-bootstrap defaults to .navbar-expand), grids are auto-fill, and filter rows flex-wrap, so content degrades rather than breaking. The login prompt's fixed 960px width becomes a max-width so the auth pages - the first screen on small viewports - no longer overflow. Wider tables can still overflow their page; full responsive pass is tracked separately (/impeccable adapt). Found by /impeccable audit (P1).
The contrast fix for selected values and input text relied on react-select emotion defaults surviving upgrades; make the light token explicit. Addresses code-review deviation on audit P1-2.
The deleted-image placeholder is now a .DeletedImage element instead of a srcless <img alt=Deleted>, so query for it directly and also assert its label. Test intent (placeholder renders for null entries) is unchanged. Fixes CI failure in PR stashapp#1218.
Collaborator
|
Please split unrelated fixes up into separate PRs. Makes them much easier to review and merge. |
Contributor
Author
|
I tried to keep the commits in atomic form. Comments are well written too but if you still find it hard to review, of course I can split. How do you want me to split? |
Collaborator
|
Based on the PR description I would expect four PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Auth redirect called during render (
Login.tsx)if (isAuthenticated) navigate("/")ran in the component body,triggering React updates during render and risking a loop while auth
state resolves. Replaced with a declarative
<Navigate to="/" replace />returned after all hooks.
2. react-select contrast failures (
App.scss,studioSelect/styles.scss)color: blackonthe dark
$secondarycontrol (~2.3:1). Now pinned explicitly to$text-color(~9.9:1).background; now
$dark-texton the existing$muted-graychip (~9.6:1).StudioSelect's parent-studio label usedrgba(black, 0.5)inside thevalue container, rendering near-invisible; dropped the override so it
inherits the legible
$text-muted.All select surfaces (filters, forms, search) now meet WCAG AA text contrast.
3. Deleted images invisible in edit diffs
Removed images were rendered as srcless
<img alt="Deleted">tags, whichbrowsers draw as zero-width boxes — edit diffs showed blank gaps where
removed images should appear. Added a shared
DeletedImagefragment(muted tile, X icon, "Deleted" label, matching the existing
empty-thumbnail language) and used it in
ImageChangeRowandAmendableImageChangeRow.4. Global 1210px viewport lockout (
theme.scss,App.scss)body { min-width: 1210px }— carried over from the Bootstrap 5migration — forced horizontal scrolling of the entire app on any narrower
viewport. Removed it: the navbar already wraps (
react-bootstrapdefaults to
.navbar-expand), grids auto-fill, and filter rowsflex-wrap. The login prompt's fixed
960pxwidth becamemax-widthsoauth pages no longer overflow as the first screen seen at smaller sizes.
Wide tables can still overflow their own page; a full responsive pass is
out of scope here.