sensei icon indicating copy to clipboard operation
sensei copied to clipboard

Update font sizes for module title and lesson header

Open Imran92 opened this issue 1 year ago β€’ 6 comments

Resolves https://github.com/Automattic/themes/issues/7085

Proposed Changes

  • Updates font size of lesson header and Module title in Course theme

We've used the preset font sizes of course theme. We've used the medium font size for the module name and small font size for the lesson title.

For default variation of Course theme, we didn't change the font size as the medium of default was a bit smaller.

Testing Instructions

  1. Checkout this branch of Course theme https://github.com/Automattic/themes/pull/7465
  2. Create a Course, in Course Outline block, add a few modules and lessons
  3. Make sure the Lesson title looks bigger
  4. Make sure the Module name is bigger than Lesson title
  5. Check for all variations of the Course theme
  6. Check with other themes to make sure nothing has broken with them.

Screenshots:

Current state Screenshot 2023-11-07 at 8 08 07 AM Screenshot 2023-11-07 at 8 08 29 AM Screenshot 2023-11-07 at 8 08 51 AM

Pre-Merge Checklist

  • [x] PR title and description contain sufficient detail and accurately describe the changes
  • [x] Acceptance criteria is met
  • [ ] Decisions are publicly documented
  • [x] Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • [x] All strings are translatable (without concatenation, handles plurals)
  • [x] Follows our naming conventions (P6rkRX-4oA-p2)
  • [ ] Hooks (p6rkRX-1uS-p2) and functions are documented
  • [x] 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 avatar Aug 08 '23 14:08 Imran92

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 83e8edacedc39888d935ef3dafb03140c0cc3dc9 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/style.js wp-polyfill 24 B +24 B ( +100% πŸ”Ό )

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

github-actions[bot] avatar Aug 08 '23 14:08 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 51.93%. Comparing base (2c5b426) to head (83e8eda). Report is 1 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7068   +/-   ##
=========================================
  Coverage     51.93%   51.93%           
  Complexity    11277    11277           
=========================================
  Files           631      631           
  Lines         47710    47710           
  Branches        421      421           
=========================================
  Hits          24779    24779           
  Misses        22594    22594           
  Partials        337      337           

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 28db587...83e8eda. Read the comment docs.

codecov[bot] avatar Aug 08 '23 14:08 codecov[bot]

@Imran92 I'm not sure that this should be configurable via a block setting for a few reasons:

  1. It's a bit weird that you can set the Lessons subheading font size but not the module font size.
  2. It makes it possible to set the Lessons subheading font size to be larger than the module font size, which we probably don't want to enable since the module is an <h2> and the Lessons subheading is an <h3>.
  3. Changes to the UI should really go through design first for feedback.

There are already some notes in the issue description where Andrei left some design feedback:

We could easily bump it up to 20px, and increase the padding top & bottom to 10px, instead of 5 right now.

I added a consideration as well:

We should be careful not to make the "Lessons" text bigger than the name of the module. For example, the module name for the Blue variation has a font size of 19.8px, so changing "Lessons" text to 20px would make it larger than the module, which is not what we want. Given this, we'll likely need to bump the font size of the module proportionally as well.

donnapep avatar Aug 12 '23 22:08 donnapep

Another thought that may be better - We could obey the heading font sizes that are in the original designs. So the module (<h2>)nwould be 48px and Lessons subheading (<h3>) would be 36px on desktop.

If you like, we could move this one back to To Do given there are higher priority things now. πŸ™‚

donnapep avatar Aug 12 '23 22:08 donnapep

Thanks for taking a look @donnapep πŸ‘‹

Another thought that may be better - We could obey the heading font sizes that are in the nf4oV51c1JBxJKXshMe7cd-fi-20020042%3A412. So the module (<h2>)nwould be 48px and Lessons subheading (<h3>) would be 36px on desktop.

I tried using the default sizes of h2 and h3 (48px and 36px), it looked a bit too big to me. I'm attaching two screenshots below, let me know what you think

h2 and h3 Screenshot 2023-11-07 at 7 48 51 AM

We could easily bump it up to 20px, and increase the padding top & bottom to 10px, instead of 5 right now.

I went for this approach. basically I'm using the preset font sizes of Course theme now. I'm using the medium size for the module name and small for the lesson title, in default variation, small is 20px. I've also increased the padding to 10px.

We should be careful not to make the "Lessons" text bigger than the name of the module. For example, the module name for the Blue variation has a font size of 19.8px, so changing "Lessons" text to 20px would make it larger than the module, which is not what we want. Given this, we'll likely need to bump the font size of the module proportionally as well.

As we are using the medium and small presets now, that takes care of this issue as small is smaller than medium.

Imran92 avatar Nov 07 '23 03:11 Imran92

Could we change the default font size for modules from 1.1em to 20px, and "Lessons" from 11px to 14px? It's a bit too small on other themes.

Good suggestion, I've updated them here https://github.com/Automattic/sensei/pull/7068/commits/d3e9c3a4b7e7a81d2f291df9c53688f645578131. I've used rem units for the font sizes.

Imran92 avatar Nov 08 '23 09:11 Imran92

Test the previous changes of this PR with WordPress Playground.

github-actions[bot] avatar Mar 14 '24 18:03 github-actions[bot]