Skip to content
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

fix: text visibility issues in error page in dark mode #30408

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

georgewrmarshall
Copy link
Contributor

@georgewrmarshall georgewrmarshall commented Feb 18, 2025

Description

This PR addresses an issue where the error page text was not visible in dark mode due to missing theme handling and color inheritance. The changes ensure proper text visibility regardless of the user's system theme by:

  1. Adding TextColor.inherit to all text components in the error page to ensure proper color inheritance
  2. Fixing the theme handling in base styles to properly apply dark mode colors
  3. Ensuring the banner alert text inherits the correct color
  4. Adding max-width to textarea components to prevent overflow issues
  5. Adding comprehensive Storybook stories for the error page component to improve test coverage and enable isolated development

Open in GitHub Codespaces

Related issues

Fixes: #30407

Manual testing steps

  1. Set your system to dark mode
  2. Trigger an error in MetaMask by setting if (error) { to if (true) { in ui/pages/index.js
  3. Verify the error page text is visible and properly contrasted
  4. Switch to light mode and repeat steps 2-3
  5. Test the Textarea component to ensure it doesn't overflow its container
  6. Run Storybook and verify all error page stories render correctly

Screenshots/Recordings

Before

error.page.bug720.mov

After

after720.mov
storybook.after720.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Feb 18, 2025
@georgewrmarshall georgewrmarshall self-assigned this Feb 18, 2025
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 18, 2025
@@ -1,6 +1,8 @@
.mm-textarea {
$resize: none, both, horizontal, vertical, initial, inherit;

max-width: 100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes issue with Textarea component extending beyond container

Before

Screenshot 2025-02-16 at 10 11 19 AM

After

Screenshot 2025-02-18 at 8 58 19 AM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing !


html,
body {
Copy link
Contributor Author

@georgewrmarshall georgewrmarshall Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to include body here so it inherits the color styles for text at the moment we are setting it to text/default which defaults to grey

after.body720.mov

@@ -110,16 +110,27 @@ const ErrorPage: React.FC<ErrorPageProps> = ({ error }) => {
size={IconSize.Xl}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page will be rendered outside of the theme provider so we need to ensure it works by changing all text components to inherit the text color from html/body

@metamaskbot metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 18, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [ed2034e]
Page Load Metrics (1816 ± 130 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32026921678513246
domContentLoaded153526831792251120
load154427771816271130
domInteractive257437136
backgroundConnect894262412
firstReactRender1593392612
getState45419189
initialActions00000
loadScripts10911879129218288
setupStore86019189
uiStartup178931282079289139
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 205 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@georgewrmarshall georgewrmarshall marked this pull request as ready for review February 18, 2025 18:30
@georgewrmarshall georgewrmarshall requested review from a team as code owners February 18, 2025 18:30
@DDDDDanica
Copy link
Contributor

LGTM !

Copy link
Contributor

@vinnyhoward vinnyhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@georgewrmarshall georgewrmarshall added this pull request to the merge queue Feb 19, 2025
Merged via the queue into main with commit c1f1f6b Feb 19, 2025
87 checks passed
@georgewrmarshall georgewrmarshall deleted the fix/error-page-dark-mode branch February 19, 2025 19:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2025
@metamaskbot metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.14.0 Issue or pull request that will be included in release 12.14.0 team-design-system All issues relating to design system in Extension
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Error page text visibility issues in dark mode
5 participants