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

Adjust course theme spacings and alignments #6854

Merged

Conversation

gabrielcaires
Copy link
Contributor

@gabrielcaires gabrielcaires commented May 2, 2023

Resolves #6834

Proposed Changes

  • Move some opinionated style from the button.scss file to the file learning-mode-compat.scss
  • Update course theme styles to follow the new designs

Testing Instructions

  1. Go to the Branch
  2. npm run start
  3. Active the "Automattic/Course" theme at least version 1.2.4
  4. Check if the layout matches the design files

Extra Notes

There are opportunities to refactor the file using SCSS selector, but to avoid conflicts with other cards, I avoid doing it now.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Decisions are publicly documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari

@github-actions
Copy link

github-actions bot commented May 2, 2023

WordPress Dependencies Report

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

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
home/index.js 21.1 kB +10 B ( +0.05% 🔼 )
blocks/quiz/index.js 32.2 kB -28 B ( -0.09% ⬇️ )
course-theme/blocks/index.js 13.8 kB +428 B ( +3.21% 🔼 )
css/3rd-party/themes/course/learning-mode.js wp-polyfill 24 B +24 B ( +100% 🔼 )

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

@gabrielcaires gabrielcaires changed the title Update/spacing course theme Load the Course Learning Mode file only if the user is using the course theme May 2, 2023
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #6854 (0e3b352) into feature/learning-mode-improvements (c2a3c62) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                          Coverage Diff                          @@
##             feature/learning-mode-improvements    Automattic/sensei#6854   +/-   ##
=====================================================================
  Coverage                                 47.21%   47.21%           
  Complexity                                10131    10131           
=====================================================================
  Files                                       499      499           
  Lines                                     35886    35886           
  Branches                                    283      283           
=====================================================================
  Hits                                      16945    16945           
  Misses                                    18729    18729           
  Partials                                    212      212           

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 06aec10...0e3b352. Read the comment docs.

Base automatically changed from update/learning-mode-default-spacings to feature/learning-mode-improvements May 4, 2023 17:37
@gabrielcaires gabrielcaires changed the title Load the Course Learning Mode file only if the user is using the course theme Review course theme spacings and aligments May 11, 2023
@gabrielcaires gabrielcaires marked this pull request as ready for review May 11, 2023 09:25
@gabrielcaires gabrielcaires requested a review from a team May 11, 2023 09:26
@gabrielcaires gabrielcaires linked an issue May 11, 2023 that may be closed by this pull request
@gabrielcaires gabrielcaires changed the title Review course theme spacings and aligments Adjust course theme spacings and aligments May 15, 2023
@gabrielcaires
Copy link
Contributor Author

@merkushin the following points were fixed.

There is an issue with the spacing on the. I am suspecting it is caused by a " display: flow-root;" that is set here. I still not have sure how it works, but I suspect that removing it will requires to rewrite part of the styles 😢 .

@Imran92
Copy link
Contributor

Imran92 commented May 16, 2023

Getting an error building the sass locally, looks like a brace issue
image

@gabrielcaires
Copy link
Contributor Author

Getting an error building the sass locally, looks like a brace issue image

fixed @Imran92

@Imran92
Copy link
Contributor

Imran92 commented May 17, 2023

I've checked it against all our variations of Course theme, it looks pretty good and matches pretty well. I'm attaching some screenshots. The mismatches in the content area is mostly due to difference of post content and video dimension. But the spacings are mostly uniform

Dark:
Spacing-dark

Blue:
Spacing-blue-1

Gold:
Spacing-gold

@merkushin
Copy link
Member

In general it looks really good.

I found the only issue with the spacing above the lesson title (or the module name if it exists), it should be 56px (3.5rem).
Here it is 48px.
CleanShot 2023-05-18 at 00 28 27@2x
CleanShot 2023-05-18 at 00 28 03@2x

I also found an issue with buttons, but we have a separate card for buttons in progress.
CleanShot 2023-05-18 at 00 38 32@2x

@gabrielcaires
Copy link
Contributor Author

@merkushin Fixed here 5e45aa9

Screenshot 2023-05-18 at 11 28 19

I just noticed I am seeing different results from you. On your screenshot, the top was 48px, and on my side was 52 (both incorrect but different 🤔 ).

Same for the buttons.

Screenshot 2023-05-18 at 11 34 23

@Imran92 could you please review it in your env to double-check it?

@Imran92
Copy link
Contributor

Imran92 commented May 18, 2023

It looks pretty good to me and matches closely with all variations in my env.

Blue match
Dark match
Default match
Screen Shot 2023-05-18 at 7 20 28 PM

There are two issues I think we can address if possible:

The space on the left between the lesson content and the course navigation panel is a bit smaller. I think we need to make our content space narrower in the desktop version. In mobile, it's good. I'm talking about the space here
image

The spacing before the course title in mobile can be improved
image

@merkushin
Copy link
Member

@gabrielcaires

Checked again.
Buttons and the space in the top are okay now 👍
CleanShot 2023-05-19 at 15 03 46@2x
CleanShot 2023-05-19 at 15 05 40@2x

CleanShot 2023-05-19 at 15 04 54@2x CleanShot 2023-05-19 at 15 05 20@2x

@merkushin
Copy link
Member

@Imran92

The spacing before the course title in mobile can be improved

I think the difference here is because of the mobile status bar in the design that we don't see in our browser, so we need to "apply" it manually.

@merkushin
Copy link
Member

@Imran92
I updated the content width here: 0e3b352

In our design, we have 830px for all themes.

@Imran92
Copy link
Contributor

Imran92 commented May 20, 2023

I updated the content width here: https://github.com/Automattic/sensei/commit/0e3b35277cd80ac30101525a8d449fce5a87c0aa
Thanks @merkushin !

Another thing that I've noticed is #6897 is probably an issue caused by this branch. Should it also be fixed here?

@gabrielcaires
Copy link
Contributor Author

I updated the content width here: https://github.com/Automattic/sensei/commit/0e3b35277cd80ac30101525a8d449fce5a87c0aa Thanks @merkushin !

Another thing that I've noticed is #6897 is probably an issue caused by this branch. Should it also be fixed here?

Nice catch!
I think we should merge this PR because the premium templates were not on the card scope and another PR only for fix that, because the scope of this PR is too wide.

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 We can address the outstanding issue in another PR

@gabrielcaires gabrielcaires merged commit 29efe1a into feature/learning-mode-improvements May 22, 2023
@gabrielcaires gabrielcaires deleted the update/spacing-course-theme branch May 22, 2023 10:21
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.

Review the learning mode spacings and alignment on course theme
3 participants