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

Add stylesheets for course theme variations #7256

Merged
merged 7 commits into from Nov 14, 2023

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Nov 2, 2023

Resolves the issue of having to write css styles in variation json files.

Proposed Changes

  • Based on the new ability of Course theme of adding body class for variation and firing hook for body class, we're adding stylesheets in sensei that gets loaded based on the selected variation.

Testing Instructions

  1. Make sure you are on latest course theme trunk
  2. If you are running "npm run start", stop and restart it
  3. Select a variation of course theme
  4. Add some random style in the stylesheet applicable for that variation for testing purposes (like * { color: red !important; })
  5. Check if the style gets applied on the frontend as expected.

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

@Imran92 Imran92 added this to the 4.20.0 milestone Nov 2, 2023
@Imran92 Imran92 requested a review from a team November 2, 2023 09:46
@Imran92 Imran92 self-assigned this Nov 2, 2023
Copy link

github-actions bot commented Nov 2, 2023

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit d83b818 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
css/3rd-party/themes/course/default.js wp-polyfill 24 B +24 B ( +100% 🔼 )
css/3rd-party/themes/course/blue.js wp-polyfill 24 B +24 B ( +100% 🔼 )
css/3rd-party/themes/course/gold.js wp-polyfill 24 B +24 B ( +100% 🔼 )
css/3rd-party/themes/course/dark.js wp-polyfill 24 B +24 B ( +100% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #7256 (a65f405) into trunk (04d4056) will increase coverage by 0.02%.
Report is 1 commits behind head on trunk.
The diff coverage is n/a.

❗ Current head a65f405 differs from pull request most recent head d83b818. Consider uploading reports for the commit d83b818 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7256      +/-   ##
============================================
+ Coverage     50.83%   50.86%   +0.02%     
+ Complexity    11033    11022      -11     
============================================
  Files           611      610       -1     
  Lines         46589    46552      -37     
  Branches        402      402              
============================================
- Hits          23683    23678       -5     
+ Misses        22579    22547      -32     
  Partials        327      327              

see 8 files with indirect coverage changes


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 a4de005...d83b818. Read the comment docs.

@donnapep
Copy link
Collaborator

donnapep commented Nov 2, 2023

I'm not sure we should do this until we actually have some variation CSS. Otherwise, we're just loading an empty CSS file. I let this slide for the Course theme, but I'm calling it out now that we want to do it again. 🙂

But, I think we also need to discuss when we should start using these files. Do we think we can start now given that we just released the Course theme with the change to add the variation to the body class? The next release is currently scheduled for November 15th (for the final frontend improvements changes). Is that too soon do you think?

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 3, 2023

Hi Donna 👋 Thanks for taking a look into this

I'm not sure we should do this until we actually have some variation CSS. Otherwise, we're just loading an empty CSS file. I let this slide for the Course theme, but I'm calling it out now that we want to do it again.

Makes sense. I wanted to add these in an early PR because we may need it in the upcoming issues which will likely involve styling in Course theme and its variations (#7241, #7242, #7243 etc), so that we don't need to redo these in them and can use this PR as the base.

But, I think we also need to discuss when we should start using these files. Do we think we can start now given that we just released the Course theme with the change to add the variation to the body class? The next release is currently scheduled for November 15th (for the final frontend improvements changes). Is that too soon do you think?

I think we're ready to use these already for the next release and can start writing the sensei-block-specific styles that we currently write in the 'css' propert in the theme.json or <>.json files in these scss files instead. Because I'm not sure if there is any additional benefit of adding those css in the json files now anymore.

@donnapep
Copy link
Collaborator

donnapep commented Nov 4, 2023

I wanted to add these in an early PR because we may need it in the upcoming issues which will likely involve styling in Course theme and its variations (#7241, #7242, #7243 etc), so that we don't need to redo these in them and can use this PR as the base.

OK, maybe for now we can keep this PR open, and merge it once we have some variation CSS we want to add?

I think we're ready to use these already for the next release

An FYI that it seems likely we'll have a patch release next week to address those WP 6.4 compatibility issues.

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 6, 2023

OK, maybe for now we can keep this PR open, and merge it once we have some variation CSS we want to add?

Sounds good! 👍 I've used a variation CSS file in this PR #7263 .

An FYI that it seems likely we'll have a patch release next week to address those WP 6.4 compatibility issues.

Cool, thanks for letting me know!

@merkushin
Copy link
Member

Hi Imran 👋

Sounds good! 👍 I've used a variation CSS file in this PR #7263 .

I checked the PR and didn't find any usages of these variation CSS-files. Is this comments outdated? Or maybe it was supposed to be a link to a different PR?

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 8, 2023

Hi @merkushin 👋 Thanks for taking a look.

I checked the PR and didn't find any usages of these variation CSS-files. Is this comments outdated? Or maybe it was supposed to be a link to a different PR?

Yap right, it got outdated. Now variation files are being used in a couple of other open PRs
https://github.com/Automattic/sensei/pull/7264/files
https://github.com/Automattic/sensei/pull/7268/files

@donnapep donnapep modified the milestones: 4.20.0, 4.19.2 Nov 9, 2023
@Imran92 Imran92 requested review from a team and removed request for a team November 13, 2023 00:48
@merkushin
Copy link
Member

So, as I can see, currently we have usage for the dark and for the default variations.
Should we remove other variations from this PR if I got it right from @donnapep comment?

OK, maybe for now we can keep this PR open, and merge it once we have some variation CSS we want to add?

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 14, 2023

I don't have a strong preference for this, but I think if it's not too much of a problem, we can keep the variation CSS files for future use. We already have CSS styles in their respective .json files, but just didn't get the time to shift them here.

If we decide to remove the yet unused ones, we'd need to update the code here to check for the existence of the files before enqueueing. Also, then it'll fall upon future individual PRs to include those files when needed, and also remove the existence check once we have files for all variations because it won't be necessary then.

LMKWYT :)

Copy link
Member

@merkushin merkushin left a comment

Choose a reason for hiding this comment

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

As we discussed in the meeting, it's fine to merge it with empty styles. Works well.

@Imran92 Imran92 merged commit 97f952a into trunk Nov 14, 2023
22 checks passed
@Imran92 Imran92 deleted the add/stylesheets-for-course-theme-variations branch November 14, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants