feat: increase text and interactive elems contrast#2621
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds a selectable foreground theme setting with muted, standard, and contrast modes. It wires the setting through theme tokens, persisted app state, prehydration, settings UI, command palette navigation, translations, and tests. ChangesForeground Theme Feature
Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant useForegroundTheme
participant document.documentElement
participant localStorage
SettingsPage->>useForegroundTheme: select foreground theme
useForegroundTheme->>document.documentElement: set data-fg-theme
useForegroundTheme->>localStorage: persist preferredForegroundTheme
localStorage->>document.documentElement: prehydrate saved theme
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/Settings/FgThemePicker.vue`:
- Around line 8-11: The selector construction using
settings.preferredForegroundTheme (variable id) can throw if id contains invalid
characters; update the prehydrate logic in FgThemePicker.vue to validate or
sanitize id before passing it to el.querySelector: either check that id exists
in the canonical list of theme ids used by this component (e.g., the array/prop
that holds available theme ids) and only build the selector when it matches, or
call CSS.escape(id) to safely escape the value before interpolation; ensure you
reference and use the same identifier names (settings.preferredForegroundTheme /
id and the el.querySelector(...) call) when applying the validation/sanitization
so querySelector cannot be given a malformed selector.
In `@app/utils/prehydrate.ts`:
- Around line 42-46: preferredForegroundTheme is read from settings and applied
directly to document.documentElement.dataset.fgTheme; validate it first against
the allowed set ["muted","standard","contrast"] before assigning. Update the
logic around settings.preferredForegroundTheme in prehydrate.ts to: check that
preferredForegroundTheme is a string and is one of the allowed ids (e.g., via a
small const ALLOWED_FG_THEMES = new Set([...]) or array.includes), and only then
set document.documentElement.dataset.fgTheme = preferredForegroundTheme;
otherwise skip the assignment (or fall back to a safe default).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6954aee-32cc-45e6-ac0c-22c64796aaad
📒 Files selected for processing (21)
app/assets/main.cssapp/components/AppHeader.vueapp/components/Button/Base.vueapp/components/Input/Base.vueapp/components/Select/Base.vueapp/components/Settings/FgThemePicker.vueapp/composables/useCommandPaletteGlobalCommands.tsapp/composables/useSettings.tsapp/pages/settings.vueapp/types/command-palette.tsapp/utils/prehydrate.tsi18n/locales/en.jsoni18n/schema.jsonlunaria/styles.tsshared/utils/constants.tstest/e2e/hydration.spec.tstest/nuxt/a11y.spec.tstest/nuxt/components/CommandPalette.spec.tstest/nuxt/components/HeaderConnectorModal.spec.tstest/nuxt/composables/use-command-palette-commands.spec.tsuno.theme.ts
|
Looks very clean, amazing work 🥰 One nitpicky detail I noticed: The version seems a little bit more grey than all the other text around it. Not sure if this is a11y related, but I think we could change it to be white as well (currently it only gets white on hover).
Wdyt? |
|
@trueberryless Good point, we've heard many times that the version is too unnoticeable and needs to be more visible |
|
@alexdln we still wanna merge this? If so I can rebase and review 😄 |
|
at least in my opinion this should be merged eventually as it does improve accessibility. the only reason why I hadn't reviewed this pr was because of the concerns that coderabbit had raised. |
|
@ghostdevv thank you for the bump, ready for review :) |
5 participants
🔗 Linked issue
Resolves #1510
🧭 Context
One of the issues with the a11y and a recurring topic with the community is poor text visibility. I've also experienced this myself. So, I'm trying to increase contrast. As usual, I did it quite carefully - changed the colors by 10% and added a new shade for the borders (I use it for inputs/selects and in the midtone for buttons [I think they need a redesign to make them more visible, not the borders])
I also added foreground shade select so that the user can choose the desired format, where the default contrast is slightly increased from the previous one and we can switch to the old one or to an even higher contrast one