fix(accesibility): use rem for font sizes ((WCAG 1.4.4) — issue #1024) #2946
Conversation
IsraelMancha
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid,
karenvicent and
mohicody
as code owners
WalkthroughCSS font-size units were converted from px to rem across root variables, specific progress bar text, and several mobile auth page headings. No structural or functional code changes beyond unit updates and added comments in index.css. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/package.json (1)
61-61: Remove unused Rollup dependency (client/package.json)Search output shows "rollup" only in client/package.json and package-lock.json; no rollup.config.* files, scripts, or imports were found — remove the dependency.
- "rollup": "^4.50.1",client/src/Components/ProgressBars/index.css (1)
5-7: Use a design token instead of a literal rem.Keeps typography consistent with the new scale.
-.progress-bar-container p.MuiTypography-root:has(span) { - font-size: 0.75rem; -} +.progress-bar-container p.MuiTypography-root:has(span) { + font-size: var(--env-var-font-size-small-plus); +}client/src/Pages/Auth/index.css (1)
106-111: Prefer tokens over hard-coded 1.125rem.Introduce a token for 18px to avoid magic numbers and ease future scale changes.
Change here:
.forgot-password-page h1, .check-email-page h1, .password-confirmed-page h1, .set-new-password-page h1 { - font-size: 1.125rem; + font-size: var(--env-var-font-size-large-minus); }And add the token in client/src/index.css (see separate comment).
client/src/index.css (1)
42-49: Add a token for 18px to support mobile h1 usage.Completes the scale and removes the remaining hard-coded 1.125rem.
--env-var-font-size-large: 1rem; /* 16px */ --env-var-font-size-large-plus: 1.375rem; /* 22px / 16 */ --env-var-font-size-xlarge: 1.875rem; /* 30px / 16 */ + --env-var-font-size-large-minus: 1.125rem; /* 18px / 16 */
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/package.json(1 hunks)client/src/Components/ProgressBars/index.css(1 hunks)client/src/Components/WalletProvider/index.css(1 hunks)client/src/Pages/Auth/index.css(1 hunks)client/src/index.css(1 hunks)
🔇 Additional comments (2)
client/src/index.css (1)
42-48: Nice conversion to rem—LGTM.Accurate px→rem conversions, aligned with WCAG 1.4.4 goals.
client/src/Components/WalletProvider/index.css (1)
27-27: Drop !important from .wallet-adapter-modal-title; confirm CSS variablesFile: client/src/Components/WalletProvider/index.css (around line 27)
-.wallet-adapter-modal-title { - font-size: var(--env-var-font-size-medium-plus) !important; -} +.wallet-adapter-modal-title { + font-size: var(--env-var-font-size-medium-plus); +}
- rg output shows spacing tokens used with !important in this file: line 33:
margin: var(--spacing-md) 0 !important;and line 51:margin: var(--spacing-sm) 0 !important;.- No global definitions were found for
--spacing-sm,--spacing-md, or--env-var-font-size-medium-plus; confirm these are declared in a global :root/theme file.- If overriding third‑party styles requires !important, keep it; otherwise remove to improve maintainability.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR successfully converts font sizes to rem units for WCAG compliance but includes an unrelated dependency addition and minor issues that could be improved for better maintainability and clarity.
🌟 Strengths
- Correct rem conversions enhance accessibility and respect user preferences.
- Comprehensive changes across multiple components ensure consistency.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | client/package.json | Maintainability | Unrelated dependency increases bloat | |
| P2 | client/src/Components/WalletProvider/index.css | Architecture | Semantic variable swap may break hierarchy | |
| P2 | client/src/index.css | Correctness | Missing html font-size limits scaling | |
| P2 | client/src/index.css | Documentation | Comment misstates rem as fixed px | |
| P2 | client/src/Components/ProgressBars/index.css | Correctness | Proper rem conversion enhances compliance |
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: client/src/index.css
Speculative: The rem conversions assume a 16px base font size (standard browser default), but without explicitly setting font-size: 100% on the html element, user browser preferences may not be fully respected. This limits the accessibility benefits of rem units.
Suggestion:
Add `html { font-size: 100%; }` to ensure proper scaling relative to user's browser settings.Related Code:
:root {
--env-var-font-size-small: 0.6875rem; /* 11px / 16 */
--env-var-font-size-small-plus: 0.75rem; /* 12px / 16 */
--env-var-font-size-medium: 0.8125rem; /* 13px / 16 */
}💡 LlamaPReview Community
Have feedback on this AI Code review tool? Join our GitHub Discussions to share your thoughts and help shape the future of LlamaPReview.
client/package.json
Outdated
| "eslint-plugin-react-hooks": "^5.0.0", | ||
| "eslint-plugin-react-refresh": "^0.4.6", | ||
| "prettier": "^3.3.3", | ||
| "rollup": "^4.50.1", |
There was a problem hiding this comment.
P2 | Confidence: High
The addition of the Rollup dependency is unrelated to the stated goal of converting font sizes to rem units. This introduces unnecessary bloat and maintenance overhead for a PR focused solely on accessibility improvements.
| "rollup": "^4.50.1", | |
| Remove this line as it's unrelated to the PR's objective. |
There was a problem hiding this comment.
Unrelated change, please do remove
client/src/index.css
Outdated
| --env-var-font-size-small: 0.6875rem; /* 11px / 16 */ | ||
| --env-var-font-size-small-plus: 0.75rem; /* 12px / 16 */ | ||
| --env-var-font-size-medium: 0.8125rem; /* 13px / 16 */ | ||
| --env-var-font-size-medium-plus: 0.875rem; /* 14px / 16 */ |
There was a problem hiding this comment.
P2 | Confidence: High
The comment incorrectly states 1rem equals 16px. While true at default browser settings, rem units are relative to the root font size, which may be changed by user preferences. The comment should clarify this relative nature to avoid future misunderstandings.
| --env-var-font-size-medium-plus: 0.875rem; /* 14px / 16 */ | |
| Update comment to: `/* 1rem (relative to root font size) */` |
| } | ||
| .progress-bar-container p.MuiTypography-root:has(span) { | ||
| font-size: 12px; | ||
| font-size: 0.75rem; |
There was a problem hiding this comment.
P2 | Confidence: High
The conversion from 12px to 0.75rem is mathematically correct (12/16=0.75) and maintains the same visual size while now respecting user font preferences. This change properly implements the WCAG 1.4.4 requirement.
There was a problem hiding this comment.
@gorkemcetin your thoughts here?
There was a problem hiding this comment.
I am good with this if that is the case.
|
|
||
| .wallet-adapter-modal-title { | ||
| font-size: var(--font-size-h5) !important; | ||
| font-size: var(--env-var-font-size-medium-plus) !important; |
There was a problem hiding this comment.
P2 | Confidence: Medium
This change replaces a semantic variable (--font-size-h5) with a specific size variable. While consistent with the rem conversion goal, it may break the design system's semantic hierarchy if --font-size-h5 was part of a structured heading scale. The new variable might not maintain the intended visual relationship with other heading levels.
Code Suggestion:
Either retain the original semantic variable (if it's being converted elsewhere) or ensure all heading variables are consistently converted to maintain design system integrity.
There was a problem hiding this comment.
Hello,
Thanks for your contribution here! @gorkem-bwl is the authority on styling here, so he'll make the call about changing the font sizes.
Please remove all the package and package-lock changes; bumping dependencies should be done in a separate PR if there is a need to do so.
Thank you!
|
@IsraelMancha we need a few screenshots about the new font sizes. I'll eventually test it but I have to make sure we are on the same page by looking at the screenshots, and see what has changed. |
|
Closing due to inactivity |
3 participants
Updated font sizes across components from
pxtoremto improve scalability, accessibility, and consistency with the theme.Fixes #1024
<div>Add</div>, use):