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

Update font sizes for module title and lesson header #7068

Merged
merged 31 commits into from
Mar 14, 2024

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Aug 8, 2023

Resolves Automattic/themes#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 Make theme variation body class available in editor screens themes#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

  • 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.16.1 milestone Aug 8, 2023
@Imran92 Imran92 requested a review from a team August 8, 2023 14:20
@Imran92 Imran92 self-assigned this Aug 8, 2023
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 83e8eda 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.

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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.

@Imran92 Imran92 changed the title Add ability to set font size for subheader Add ability to configure font size for subheader Aug 9, 2023
@donnapep
Copy link
Collaborator

donnapep commented Aug 12, 2023

@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
Copy link
Collaborator

donnapep commented Aug 12, 2023

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 donnapep removed this from the 4.16.1 milestone Aug 12, 2023
@Imran92
Copy link
Contributor Author

Imran92 commented Nov 8, 2023

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 d3e9c3a. I've used rem units for the font sizes.

@Imran92 Imran92 changed the title Add ability to configure font size for subheader Update font sizes for module title and lesson header Nov 8, 2023
@donnapep donnapep modified the milestones: 4.20.0, 4.19.2 Nov 9, 2023
Base automatically changed from add/stylesheets-for-course-theme-variations to trunk November 14, 2023 19:19
@donnapep donnapep modified the milestones: 4.19.2, 4.19.3 Nov 21, 2023
@donnapep donnapep removed this from the 4.19.3 milestone Dec 14, 2023
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.

Looks good to me. However the conflict needs to be resolved.

And I'm not entirely sure why @donnapep asked to set default values in pixels — was there something behind that or em is also fine?

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.

Just to make the required action obvious: need to resolve conflicts.

@Imran92 Imran92 added this to the 4.20.2 milestone Jan 19, 2024
@Imran92 Imran92 closed this Jan 19, 2024
@Imran92 Imran92 reopened this Jan 19, 2024
@m1r0 m1r0 modified the milestones: 4.20.2, 4.20.3 Feb 7, 2024
@merkushin merkushin modified the milestones: 4.21.0, 4.21.1 Feb 27, 2024
Copy link

Test the previous changes of this PR with WordPress Playground.

@donnapep donnapep merged commit 3b641fc into trunk Mar 14, 2024
25 checks passed
@donnapep donnapep deleted the add/ability-to-set-font-size-for-subheader branch March 14, 2024 18:43
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.

Lessons subheading is too small in Course Outline block
4 participants