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

Fix not enrolled notice style in lm #7263

Merged
merged 10 commits into from Nov 15, 2023
Merged

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Nov 3, 2023

Resolves #7241

Proposed Changes

  • Updated styles of the notice that's showed on lessons when user is not enrolled to match the other notices.

Testing Instructions

We don't have and exact design for this notice. So we've fixed it taking inspirations from other notices.

  1. Create a course with a lesson
  2. Try to access the lesson as a student without taking the course
  3. Check the notice both from Course theme and other themes
  4. Check all variations of course theme
  5. Check in mobile view

Desktop

Screenshot 2023-11-03 at 10 44 23 PM

Mobile

Screenshot 2023-11-03 at 10 44 36 PM

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.20.0 milestone Nov 3, 2023
@Imran92 Imran92 requested a review from a team November 3, 2023 16:54
@Imran92 Imran92 self-assigned this Nov 3, 2023
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #7263 (3875803) into trunk (5a54368) will not change coverage.
Report is 24 commits behind head on trunk.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7263   +/-   ##
=========================================
  Coverage     50.76%   50.76%           
  Complexity    11048    11048           
=========================================
  Files           611      611           
  Lines         46664    46664           
  Branches        402      402           
=========================================
  Hits          23687    23687           
  Misses        22650    22650           
  Partials        327      327           
Files Coverage Δ
...ludes/blocks/course-theme/class-lesson-actions.php 98.00% <100.00%> (ø)
.../course-theme/class-sensei-course-theme-lesson.php 94.81% <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 93de5b8...3875803. Read the comment docs.

@Imran92 Imran92 changed the base branch from trunk to add/stylesheets-for-course-theme-variations November 6, 2023 01:06
Copy link

github-actions bot commented Nov 6, 2023

WordPress Dependencies Report

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

No changes detected in the current commit. But the comment was left so it is possible to check for the edit history.

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

@m1r0
Copy link
Member

m1r0 commented Nov 7, 2023

It tests well on the Course theme but the notice button looks different on Astra and Divi:

image image

Is fixing the other themes in the scope of this PR?

@donnapep donnapep modified the milestones: 4.20.0, 4.19.2 Nov 9, 2023
@Imran92
Copy link
Contributor Author

Imran92 commented Nov 13, 2023

Thanks for thaking a look @m1r0 ! <3

Is fixing the other themes in the scope of this PR?

The issue was specific to Course theme, but the fix for other themes was very small, so I've addressed it here b071d7e. Can you kindly take a look again? Thanks

m1r0
m1r0 previously approved these changes Nov 14, 2023
Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Nice! Looks good.

FYI, not sure if it's related, but I've noticed that the buttons on the registration notice are not visible on Course.

image

Base automatically changed from add/stylesheets-for-course-theme-variations to trunk November 14, 2023 19:19
@Imran92 Imran92 dismissed m1r0’s stale review November 14, 2023 19:19

The base branch was changed.

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 14, 2023

Hi @m1r0 👋 Sorry the review became stale after the base branch got merged to trunk, can you kindly do it again?

Thanks!!

m1r0
m1r0 previously approved these changes Nov 14, 2023
@Imran92 Imran92 merged commit d643ee1 into trunk Nov 15, 2023
24 checks passed
@Imran92 Imran92 deleted the fix/not-enrolled-notice-style-in-lm branch November 15, 2023 15:29
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.

Style the "Take Course" notice in Learning Mode when not enrolled
3 participants