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 spacings and alignment for Divi #6883

Merged
merged 13 commits into from
May 24, 2023

Conversation

gabrielcaires
Copy link
Contributor

@gabrielcaires gabrielcaires commented May 15, 2023

Resolves #6856

Proposed Changes

  • Adjust spacings and alignment for Divi on Frontend
  • Fix basic spacing issues on the editor (Only to make it usable).
  • Load an editor css file only for divi.

Testing Instructions

  1. Install and Active the divi theme
  2. Go to a course with learning mode enabled
  3. Check if the spacings are similar to the expected designs.
Screenshot 2023-05-15 at 15 05 57 Screenshot 2023-05-15 at 15 08 50 Screenshot 2023-05-15 at 15 07 48 Screenshot 2023-05-15 at 15 10 16

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly 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

@gabrielcaires gabrielcaires changed the base branch from trunk to feature/learning-mode-improvements May 15, 2023 15:01
@gabrielcaires gabrielcaires changed the base branch from feature/learning-mode-improvements to update/spacing-course-theme May 15, 2023 15:02
@gabrielcaires gabrielcaires requested a review from a team May 15, 2023 15:02
@gabrielcaires gabrielcaires marked this pull request as ready for review May 15, 2023 15:03
@gabrielcaires gabrielcaires added this to the 4.15.0 milestone May 15, 2023
@github-actions
Copy link

github-actions bot commented May 15, 2023

WordPress Dependencies Report

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

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
js/grading-general.js 2.65 kB -4 B ( -0.16% ⬇️ )
home/index.js 21.1 kB +10 B ( +0.05% 🔼 )
blocks/quiz/index.js 32.2 kB -28 B ( -0.09% ⬇️ )
course-theme/course-theme.editor.js 2.63 kB -12 B ( -0.46% ⬇️ )
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% 🔼 )
css/3rd-party/themes/divi/learning-mode.js wp-polyfill 24 B +24 B ( +100% 🔼 )
css/3rd-party/themes/divi/learning-mode.editor.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 May 15, 2023

Codecov Report

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

Impacted file tree graph

@@                          Coverage Diff                          @@
##             feature/learning-mode-improvements    #6883   +/-   ##
=====================================================================
  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 fb9349c...8ca3169. Read the comment docs.

@merkushin
Copy link
Member

Front end looks pretty good. Here are a few issues I noticed:

Too little space between the paragraph and the video above:
CleanShot 2023-05-18 at 01 21 32@2x

No space at all between the lesson name and the module name:
CleanShot 2023-05-18 at 01 21 16@2x

By design, it is 24px:
CleanShot 2023-05-18 at 01 23 50@2x

Too much space in the navigation for lessons without modules:
CleanShot 2023-05-18 at 01 19 51@2x

Too little space on the top of the content area:
CleanShot 2023-05-18 at 01 19 17@2x

Base automatically changed from update/spacing-course-theme to feature/learning-mode-improvements May 22, 2023 10:21
@gabrielcaires
Copy link
Contributor Author

@merkushin I didn't have success reproducing the issues after I updated the branch.

Screenshot 2023-05-22 at 11 37 47 Screenshot 2023-05-22 at 14 05 40 Screenshot 2023-05-22 at 11 36 16

I tried to recreate my env to try to reproduce.

Could you please check again? I am still trying to figure out if my env is reliable.

@merkushin
Copy link
Member

@gabrielcaires
I still see some of those issues. Course navigation on mobile has changed partially to a better, partially to a worse.
I recorded my screen. I forgot to show the active theme there, but I double-checked, it is Divi.

Divi-LM-Appearance.mov

* Enqueue Course theme-specific Learning Mode styles in the admin for the Site Editor and Lesson Editor.
*/
function sensei_admin_load_learning_mode_style_for_divi_theme() {
$is_course_theme = 'divi' === wp_get_theme()->get_template();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this variable be named $is_divi_theme or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb


if ( $is_lesson_editor || $is_site_editor ) {
Sensei()->assets->enqueue(
'div-learning-mode',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be divi-learning-mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb

*/
function sensei_load_learning_mode_style_for_divi_theme() {
$course_id = Sensei_Utils::get_current_course();
$is_course_theme = 'divi' === wp_get_theme()->get_template();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this variable be named $is_divi_theme or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb

);

Sensei()->assets->enqueue(
'div-learning-mode-editor',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be divi-learning-mode-editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb

@@ -89,4 +89,54 @@ function sensei_fix_divi_learning_mode_video_template_excerpt() {
remove_filter( 'render_block_core/post-excerpt', array( ET_GB_Block_Post_Excerpt::instance(), 'render_block' ) );
}
}

/**
* Enqueue Course theme-specific Learning Mode styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Divi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb

}

/**
* Enqueue Course theme-specific Learning Mode styles in the admin for the Site Editor and Lesson Editor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Divi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here 2703ebb

@Imran92
Copy link
Contributor

Imran92 commented May 23, 2023

I've introduced a small fix here, otherwise it wasn't working for me locally bac7054

@merkushin
Copy link
Member

After Imran's fix, issues with spacing in content part on desktop were resolved.
The last issue I see is the spacing in the course navigation on mobile.

CleanShot 2023-05-23 at 13 57 43@2x
CleanShot 2023-05-23 at 13 57 35@2x

@gabrielcaires
Copy link
Contributor Author

@merkushin As I described here due to some reason that I couldn't identify, I can not reproduce it, so I removed the change I introduced based on my env. [8ca3169]
Could you please test it again?

@gabrielcaires gabrielcaires requested a review from a team May 23, 2023 18:30
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.

👍

CleanShot 2023-05-24 at 01 10 55@2x

@Imran92
Copy link
Contributor

Imran92 commented May 24, 2023

Merging into the feature branch as it's approved 🎆

@Imran92 Imran92 merged commit f301313 into feature/learning-mode-improvements May 24, 2023
19 checks passed
@Imran92 Imran92 deleted the update/review-div-spacing branch May 24, 2023 01:18
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 + Divi on Frontend & Editor: Review and adjust spacings and alignment for Divi
3 participants