-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: About page uses theme background and text colors (fixes white bottom half in dark/high-contrast mode) #3563 #3571
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
Conversation
… styled-component imports
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
@raclim Please review this PR. Thank You! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please Run tests locally once.
client/modules/About/About.styles.js
Outdated
} | ||
`; | ||
|
||
// 5. Intro Description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these comments to maintain consistency with the overall code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @yugalkaushik sir I have removed comment and running test locally. will update PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yugalkaushik sir, I have done everything now please review my PR and merge it if everything goes well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to address me with Sir. I'm just a contributor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@raclim can you please review my PR and merge if it's correct ? |
@raclim can you please review my PR?? If there is something wrong please let me know I will do my best to fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry that it took a while to get to this, thanks for putting this together!
I think the approach here ended up being a bit broader than what the original issue required. Instead of introducing a new global style, the fix ideally would be made more directly by adjusting the element’s height properties (noted in subsequent comments in the original thread). The introduction of the global style and a few other css changes seems to be introducing unrelated side effects (such as the font)—ideally, we want to keep changes to other visual elements of the About page to a minimum.
Because of the scope of changes required to get this PR ready, I don’t think we'll be able to move forward with this one. I'm sorry that we couldn't get this one in, but we really appreciate your effort and interest!
Fixes #3563
Changes:
I also solved the problem of the bottom half of the About page always having a white background, hence text difficult to read in dark and high-contrast themes.
Through the introduction of a global style that defines the background and text color for the whole page, the About page now correctly honors the chosen theme from top to bottom. This translates to improved readability and uniform appearance in all themes.
I also tidied up the imports and made sure everything employs the proper paths so that the styling is smooth.
refer to this image
@raclim
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123