-
Notifications
You must be signed in to change notification settings - Fork 614
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(CMS, Page Builder): Preview Background Color and Rich Text Color on Dark Theme #1052
Fix(CMS, Page Builder): Preview Background Color and Rich Text Color on Dark Theme #1052
Conversation
Hi @jarretmoses looks great! I'm just not entirely sure we want to have the Page preview to also render in the dark mode, this image in particular: @SvenAlHamad what do you think? This looks cool to me, but not sure if it's really usable. The site will rarely have dark mode 🤔 |
@Pavel910 that is fine if not but if you look at the last image I attached, the viewer text is unintelligible for |
@jarretmoses actually that appears to be a bug, because even in dark mode, we always had the same styles in that Preview pane as in Light mode. I'll have to double check, but this does look great like this. |
It's been a while since I last looked at this: ) We have to be a bit careful here. The background colour on that section (PageBuilder page preview) is intentionally not connected with the admin dark theme. The reason is that the page template, that is the site theme, is responsible for both font styles, font colours as well as the background colour inside the preview and not the admin theme. That being said, we can take the approach of saying that the page builder theme also has a "dark" mode, which is activated via the same way as the admin theme, by adding One thing we want to ensure is that the page builder theme is not tightly coupled with the admin theme, this means you should not use the That being said, I would suggest a few minor changes to this PR to make this work:
Hope this helps. If you have any questions, let me know, happy to help. |
2370ea2
to
5a16a96
Compare
@SvenAlHamad Thanks for the feedback. Ive made the 2 suggested changes. @Pavel910 |
@jarretmoses thanks for updating the PR. I had a quick look at 5a16a96 I believe you might be missing a definition for |
5a16a96
to
81764be
Compare
@SvenAlHamad added. You are correct that it was white |
@jarretmoses I'll review the PR this week and get back to you. We're caught in the maze of different tasks, difficult to stay prompt on all fronts. Hope you understand :) Thanks for your time 🎉 🚀 |
@Pavel910. Not a problem at all. Just here to help. |
@jarretmoses can I ask you to do one more change? I'd like these to be actual variables, which are only set if they don't already exist. Please add this, instead of hardcoded values: // dark theme
$webiny-theme-dark-surface: #242424 !default;
$webiny-theme-dark-text-primary-on-background: rgba(255, 255, 255, 0.87) !default;
body.dark-theme {
--webiny-theme-color-text-primary: #{$webiny-theme-dark-text-primary-on-background};
--webiny-theme-color-surface: #{$webiny-theme-dark-surface};
} Once this is done, I'm merging the PR. Thanks! 🚀 |
@Pavel910 done |
@jarretmoses merged! Thank you for your time! 👍 🚀 |
Related Issue
Closes #953
Your solution
There were a couple ways to fix this (I am not familiar with preferred standards), however I removed a collision the styled components were creating with enforced background colors that were still on the light theme but were redundant due to
mdc-elevation--z${VALUE}
styles. For the rich text editor I updated the value for the variable when the page is within the.dark-theme
. Was debating updating--webiny-theme-color-surface
instead of removing the styled css declarations.How Has This Been Tested?
Manually (locally) checking before and after styles based on attached screens shots. Looked through the app also to see where else these styles may have been used.
Screenshots (if relevant):