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

Review learning mode default spacings #6828

Conversation

gabrielcaires
Copy link
Contributor

@gabrielcaires gabrielcaires commented Apr 19, 2023

Fix #6785
Fix 6802

Proposed Changes

  • Update the learning mode spacings
  • Update the menu animation - it is still not the final version but we can use it as a reference to check the spacings.
  • Update blocks to render the same HTML that the frontend is rendering.
  • Fix lesson count spacing issue.
  • Add more classes to the default template block.
  • Rename some classes to use the names generated by Gutenberg.
  • Remove extraneous layout.scss

Extra Notes

  1. I updated the mobile menu animation because any other solution that I found will just require rework quickly, the animation card is still required to adjust the final details.

  2. There are a lot of opportunities to improve the CSS files. E.g. remove the mobile.scss, reduce the number of variables spread on the scss files, or rethink the scss file structure). It can be addressed in future PRs. Feel free to leave notes here about it, but they should not block the merge.

  3. I found spacing differences when the user is able to see the admin-bar( admin user/teacher user) and when the user is a regular student.

Testing Instructions

  1. npm run start
  2. Install and active Astra
  3. Enable the learning mode
  4. Reset any customization applied to the Lesson Default template.
  5. Use a student user to go to a to a lesson
  6. Compare the spacings with the Figma layout (Please ignore (Exit Course + Course Progress) positions on mobile when the menu is opened)
  7. Use a teacher/admin and check again if all is fine.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Decisions are publicly documented
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs - Except the elements that I described above.
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate

@gabrielcaires gabrielcaires changed the base branch from trunk to feature/learning-mode-improvements April 19, 2023 15:48
@github-actions
Copy link

github-actions bot commented Apr 19, 2023

WordPress Dependencies Report

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

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
course-theme/blocks/index.js 13.8 kB +436 B ( +3.27% 🔼 )

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

@gabrielcaires gabrielcaires changed the base branch from feature/learning-mode-improvements to update/learning-mode-default-template April 19, 2023 15:57
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #6828 (de40ec4) into feature/learning-mode-improvements (b66b88c) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@                           Coverage Diff                            @@
##             feature/learning-mode-improvements    #6828      +/-   ##
========================================================================
- Coverage                                 47.22%   47.22%   -0.01%     
  Complexity                                10131    10131              
========================================================================
  Files                                       499      499              
  Lines                                     35880    35884       +4     
  Branches                                    283      283              
========================================================================
  Hits                                      16945    16945              
- Misses                                    18723    18727       +4     
  Partials                                    212      212              
Impacted Files Coverage Δ
...ncludes/blocks/course-theme/class-page-actions.php 0.00% <0.00%> (ø)
...ludes/blocks/course-theme/class-lesson-actions.php 98.71% <100.00%> (ø)

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 b66b88c...de40ec4. Read the comment docs.

@gabrielcaires gabrielcaires requested a review from a team April 19, 2023 16:43
@donnapep
Copy link
Collaborator

donnapep commented May 2, 2023

Looks like this commit broke pagination:

Screenshot 2023-05-02 at 8 05 10 AM

It should look like this - 8mY8ui4zYK1Ylw2yIFi9Lx-fi-20020542_41426. See #6828 (comment) instead. To test this, add a page break block to a lesson.

@gabrielcaires gabrielcaires self-assigned this May 2, 2023
@donnapep
Copy link
Collaborator

donnapep commented May 2, 2023

It should look like this - 8mY8ui4zYK1Ylw2yIFi9Lx-fi-20020542_41426. To test this, add a page break block to a lesson.

Oh nuts. I think I got this wrong. It should actually be like this - https://a8c.slack.com/archives/C013QUH20TS/p1681735867106719?thread_ts=1681428586.082659&cid=C013QUH20TS. Hopefully you didn't "fix" that bit. 😳

@gabrielcaires
Copy link
Contributor Author

@donnapep I fixed all points and reviewed the Page/Lesson actions, including the mobile version.
I also reviewed all page/lesson actions (Complete/Completed/ Next Lesson/ Take Quiz), adding the new icon and fixing all spacing and sizing.

@gabrielcaires gabrielcaires changed the title Update learning mode default spacings Review learning mode default spacings May 4, 2023
@donnapep
Copy link
Collaborator

donnapep commented May 4, 2023

@gabrielcaires This is looking great! So nice to see the spacing and colors / typography improvements together. 🙂

Rather than testing to ensure that everything is pixel-perfect (Andrei will probably do a very good job of that anyway), I took a more holistic approach to ensure that the spacing looks good in general.

Mobile - Editor

I'm not sure what's going on, but the editor looks very broken 🙀 :

Screenshot 2023-05-04 at 11 14 29 AM

Mobile - Frontend

Please let me know if you'd prefer to turn some of these into a separate card, as I know we may not have designs for all of them.

Screenshot 2023-05-04 at 10 41 02 AM

  • Around the 1200px mark, there is no longer any margin around the content and it's too tight (same for the editor):

Screenshot 2023-05-04 at 10 35 28 AM

  • When a quiz has been completed, the notice is pretty broken:

Screenshot 2023-05-04 at 10 37 47 AM

  • The text in the Completed button is wrapping:

Screenshot 2023-05-04 at 10 59 47 AM

  • I wonder if we should try to make these buttons the same height?

Screenshot 2023-05-04 at 11 06 05 AM

  • It would be nice to have the bullets or numbers indented so that they left-align with the rest of the content:

Screenshot 2023-05-04 at 11 10 13 AM

Desktop - Editor

👍🏻

Desktop - Frontend

  • The site logo is very squished on the frontend, but looks fine in the editor. Its native size is 100 x 33, but the rendered size is 24 x 8 on the frontend, so it's hard to make out what it says:

Screenshot 2023-05-04 at 11 25 33 AM

@donnapep
Copy link
Collaborator

donnapep commented May 4, 2023

As per https://a8c.slack.com/archives/C02NWDZBL0H/p1683221347231259, we agreed to merge and fix the remaining issues in a new PR.

@gabrielcaires gabrielcaires merged commit 19593f9 into feature/learning-mode-improvements May 4, 2023
17 of 19 checks passed
@gabrielcaires gabrielcaires deleted the update/learning-mode-default-spacings branch May 4, 2023 17:37
@donnapep donnapep modified the milestones: 4.14.0, 4.15.0 May 12, 2023
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.

Standard Layout on the Frontend (Sensei default): Spacing and alignment
2 participants