-
Notifications
You must be signed in to change notification settings - Fork 1
Theming updates #346
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
base: develop
Are you sure you want to change the base?
Theming updates #346
Conversation
✅ Deploy Preview for padp-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for pss-scavenger-hunt ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for gbof-c19nyc-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for juel-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
UPDATE: After various discussions and more detailed info from Chelsea on what color combinations are possible in the designs, I've tweaked the schema to be more restrictive about what theme colors are allowed where. In general backgrounds of sections can be either (So for example, if you make a I am only like 55% happy with this setup, although I think I've got it working. It's a bit fiddly exactly what priority various CSS rules are getting applied in, and I don't have faith that there isn't some weird browser out there that would layer things differently and end up with the wrong text colors. But for now with the JUEL designs at least things are looking right to me locally. |
|
Is there an issue we can think this PR to? |
| @@ -1,11 +1,18 @@ | |||
| --- | |||
| const { alt, date, dateOptions = {}, category, title, author, slug, imageUrl, blurb, t } = Astro.props; | |||
| import clsx from "clsx"; | |||
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.
Single quotes.
| customSecondaryContent: secondaryContent, | ||
| customTertiaryContent: tertiaryContent, | ||
| customContent: layoutContent, | ||
| customContentAlternate: layoutContentAlternate, |
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.
These names are a little confusing (content alternate, alternate content, etc). Could we keep the naming more consistent with the fields in Tina? (e.g. main background, alternate background, main text, main text inverted, etc)
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.
Yeah it's tricky... I was attempting to match the language that's used in the Figma designs as much as possible, which is why I switched to content over text, but I don't have strong feelings about that either way.
The thing about "main text", "main text inverted" etc. is that those are not the same variables as these; they are the options from which these theme variables are selected. For example, imagine a site with the following theme:
Primary: light pink
Main background: dark purple
Alternate background: white
Main content (for light background): black
Main content inverse (for dark background): white
In this case (with the current naming), we'd have primaryContent = black (because the primary color is light), layoutContent = white (because the main background color is dark), layoutAlternateContent = black (because the alternate background color is light). However, if the theme were the same except the main background was light blue, then layoutContent would become black.
The reason for doing it this way is that in some cases in the designs we want to select the text color just based on whether it's dark or light (for example on these cards -- this isn't based on a theme color, we're just applying a black gradient over the image to make the light text pop:)

So in that case we do want to use contentInverse, but in most other cases we want to use the text color based on the themed background value, and so whether it's content or contentInverse will depend on the value of the background. So that's the difference between content (always dark), contentInverse (always light), and layoutContent (depends on value of colorLayout).
Sorry for the overly labored explanation, but that's the complexity I'm trying to wrangle here. The user never has to see any of this -- from the perspective of an editor in Tina, they just have to select the text color they want for dark backgrounds and for light backgrounds, and the correct one will be chosen based on the background color for each section.
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.
Fair enough. It's a complicated concept. I'm just thinking of another developer (myself included) coming in here to make adjustments, it's a little hard to follow which colors are being applied where.
| * Page builder styles | ||
| */ | ||
|
|
||
| .page-builder .bg-layout-alt, .page-builder .bg-layout-alt div { |
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.
Do we want these styles scoped to just custom pages? I would think we want the branding applied globally if we use any of these styles outside of custom pages.
In this PR
This PR bundles a bunch of small styling and config updates that were necessary in the course of building out the JUEL main site designs, including...
Brandingconfig and the CSS setup;