Add course theme variation body class

[Imran92]

Proposed Changes

• We've added a custom property in Course theme's theme json and the json setting of the variations as well. Here we parse the property value and add that to the body class. This will allow us to style the variations using CSS directly instead of having to write them in json files as strings.

Testing Instructions

1. Checkout this branch of Course theme Add body class for theme variations themes#7432
2. Select a variation of Course theme for your site from site editor and save
3. Reload from the frontend
4. Make sure the body element has the class course-theme-variation-[default/blue/gold/dark]
5. Check for all variations
6. Check with another block and non block theme to make sure nothing is broken

Pre-Merge Checklist

• PR title and description contain sufficient detail and accurately describe the changes
• Acceptance criteria is met
• Decisions are publicly documented
• Adheres to coding standards (PHP, JavaScript, CSS, HTML)
• All strings are translatable (without concatenation, handles plurals)
• Follows our naming conventions (P6rkRX-4oA-p2)
• Hooks (p6rkRX-1uS-p2) and functions are documented
• New UIs are responsive and use a mobile-first approach
• New UIs match the designs
• Different user privileges (admin, teacher, subscriber) are tested as appropriate
• Code is tested on the minimum supported PHP and WordPress versions
• User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
• "Needs Documentation" label is added if this change requires updates to documentation
• Known issues are created as new GitHub issues

[codecov]

Codecov Report

Merging #7225 (82b1322) into trunk (c1c3f54) will decrease coverage by 0.02%.
Report is 1 commits behind head on trunk.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##              trunk    #7225      +/-   ##
============================================
- Coverage     50.43%   50.42%   -0.02%     
- Complexity    10909    10913       +4     
============================================
  Files           605      605              
  Lines         45986    46000      +14     
  Branches        402      402              
============================================
  Hits          23194    23194              
- Misses        22465    22479      +14     
  Partials        327      327              

Files	Coverage Δ
includes/class-sensei.php	21.92% <0.00%> (-0.70%)	⬇️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 409ed2e...82b1322. Read the comment docs.

[donnapep]

This is a clever solution, though I spotted a couple of issues:

1. I had the Blue variation selected. After switching to the plugin and theme branch and refreshing a lesson page, it added the default class to the body, not the blue one.
2. If I rename the folder of the theme to something else, say course-test, then wp_get_theme()->get_template() returns course-test and the check fails. Maybe we can skip this check altogether and rely on the namespaced CSS variable instead. WDYT?

The discovery of #1 makes me more than a little nervous. 😅 It makes me wonder if they would need to go to the site editor to save the correct variation, which would be a non-starter. Which then made me wonder if there are other situations we haven't thought to test where the incorrect CSS class, or no CSS class, is added. Which then made me wonder if it's safer to just keep doing what we're doing until Gutenberg itself provides a solution. The CSS may be ugly in theme.json, but at least we know it works. Welcome to my brain. 😂

[Imran92]

Hi @donnapep 👋 Thanks so much for going through it, taking a deep look and sharing your thoughts.

I had the Blue variation selected. After switching to the plugin and theme branch and refreshing a lesson page, it added the default class to the body, not the blue one.

The discovery of #1 makes me more than a little nervous. 😅 It makes me wonder if they would need to go to the site editor to save the correct variation, which would be a non-starter

True, this is a valid case. Actually, this holds true for any time we add or change anything in any variation and release a new theme version. For example, when we have new style updates for learning mode for different variations, or button styles, or recently when we added styles for quizzes and questions, if the existing variation users don't go to the site editor and re-save the styles, they won't get the variation style updates. So I think there are the following scenarios to consider -

1. A user who's using a variation re-saves the variation after course theme release - No issues, best case scenario
2. A user who's using a variation but doesn't re-save, while we've introduced some style changes in sensei targeting CSS body class selector - New styles won't work, but they wouldn't have worked even if the styles were added in variation.json, so there probably is no additional issues as once a variation is selected, the style of the variation at that time is saved to the DB and later on, it's only loaded from DB.

There is another factor I think, if the user doesn't re-save, the default class is added to body. So is there a risk of imposing default variation's style on the sub variation?

I think the answer to the above is there isn't any added problem by it, because, if the user does not re-save, even though the variation.json does not get loaded, theme.json is always read directly from the file, so the new styles of default variations get already imposed now on the variation styles if the user does not re-save.

Just sharing my thoughts here, but I may have missed some scenarios. I think, just to be safe, even though hopefully there aren't any very apparent issues much different than the current behavior, we can just add the body class codes, but not remove the styles just yet and wait for a later time to do that, a bit like how we removed learning mode styles from the theme after some time. Also, as an added benefit, I think if we become able to use variation body class for targeting styles, we won't have to worry about the re-saving anymore as we currently do with variation.json styles. WDYT?

If I rename the folder of the theme to something else, say course-test, then wp_get_theme()->get_template() returns course-test and the check fails. Maybe we can skip this check altogether and rely on the namespaced CSS variable instead. WDYT?

Very good observation, it is a valid case. I've added a bit of additional check f45eebc for this, do you think this will do? Another thing I've noticed is this is how we load all our theme specific styles (Astra, Divi, Course). Should we try to add extra checks for them as well? Or should we keep them as they are considering (if) we didn't get much report of issues caused by them and consider this and edge case? Because if template path is changed, users probably will not only have the variation style, rather they may have no different learning mode style for Course theme as well.

Also, I'm thinking if we decide to add this code, should we add it in the Course theme directly instead of Sensei. Because Course theme is general purpose. So for example, if we want to add styles to some common blocks (like Post Title, etc), we'd want them to stay in Course theme's repo.

Which then made me wonder if it's safer to just keep doing what we're doing until Gutenberg itself provides a solution. The CSS may be ugly in theme.json, but at least we know it works. Welcome to my brain. 😂

Hahah 😄 True, I've thought so too a bit. I'm absolutely okay with not merging this PR, I just wanted to share the new approach and do some thinking together.

As we've discussed above, the variation.json styles only work (till now) when the user re-saves them, so re-save is still a core dependency, which is the same with this PR. I see there are two issues (here, and here) in GB to address it, not sure when they'll be addressed and make it to wp core.

[donnapep]

Actually, this holds true for any time we add or change anything in any variation and release a new theme version.

Thanks for the detailed explanation! ❤️ I don't know why I didn't clue into this, since I know that I have to resave in the editor every time I introduce a change to a variation, but I guess sometimes I'm just on auto-pilot. 🤣 This seems like such a big issue that I'm surprised they haven't fixed it yet. I guess the best we can do is ask them to resave the variation if we get complaints about it. Though I suspect people would be more likely to uninstall the theme instead. Maybe we should consider adding a message to the change log every time we update a variation?

we can just add the body class codes, but not remove the styles just yet and wait for a later time to do that

Yup, I fully agree with this and was going to suggest exactly that if we ended up merging.

I've added a bit of additional check f45eebc for this, do you think this will do?

I'll do a bit more testing and let you know, though it looks like a more reliable check now. 🙂

Another thing I've noticed is this is how we load all our theme specific styles (Astra, Divi, Course). Should we try to add extra checks for them as well?

Yup, noticed that as well. I think we should add the additional check, but probably better to do so in a separate PR to keep this one clean, or just create a new issue.

I'm thinking if we decide to add this code, should we add it in the Course theme directly instead of Sensei. Because Course theme is general purpose. So for example, if we want to add styles to some common blocks (like Post Title, etc), we'd want them to stay in Course theme's repo.

I don't quite follow this bit. The CSS we're loading there is for Learning Mode, which is part of Sensei, so for me it doesn't make sense to put it in Course.

There is still a bit of feedback that I don't believe has been addressed - #7225 (review) and #7225 (review).

[Imran92]

Thanks so much for for going through the PR again. I've addressed the remaining comments too ☺️

but probably better to do so in a separate PR to keep this one clean, or just create a new issue.

Thanks, I'll create a new issue for it.

I don't quite follow this bit. The CSS we're loading there is for Learning Mode, which is part of Sensei, so for me it doesn't make sense to put it in Course.

Sorry I didn't explain it better, it's just a thought I wanted to share, nothing I'm too sure about or inclined to.

Variation-specific CSS styles can need to be written for any block, for the core blocks too. The course theme can be used without Sensei too. For example, let's say we have to write some CSS which is different for every variation of the Course theme for the Query Loop block. So if the course theme itself adds the body class (we move the maybe_add_course_theme_variation_class function to the Course repo PR), then we won't need to write the CSS for the Query Loop block in each theme.json, rather we can just write the CSS for the query loop block here targeting the body class of the variation. And from Sensei, we need to just target the body class. Also, maybe we can share our workaround with the theme team too in that case if you think it can be useful. WDYT?

[donnapep]

if the course theme itself adds the body class (we move the maybe_add_course_theme_variation_class function to the Course repo PR),

This sounds like a good idea. Let's do it! 👍🏻 When making the changes, could we shorten the class names to just is-default, is-blue etc. to match the naming that may end up being used in WordPress/gutenberg#43405? We could also share this idea on that issue as you've suggested. 🙂

rather we can just write the CSS for the query loop block here targeting the body class of the variation.

When we get there, I think it would be better to have the CSS for each variation in its own CSS file(s) that would be conditionally loaded, so that we're not loading CSS that is never going to be used.

[Imran92]

Moved the functionality to Automattic/themes#7432. So closing this. Relevant issue created here #7248