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 notice style for not logged in users in learning mode #7264

Merged
merged 16 commits into from Nov 16, 2023

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Nov 3, 2023

Resolves #7242

Proposed Changes

  • Updated styles of the notice that's shown to the user on the lesson page in learning mode when the user is not logged in.

Testing Instructions

We don't have an exact design for this notice, so we followed the other notices we have that has two buttons.

  1. Create a Course with a lesson, learning mode should be enabled for the lesson
  2. Try to access the lesson without logging in
  3. Make sure the notice looks as expected
  4. Check with all variations of course theme
  5. Check with other themes
  6. Check in mobile view

Desktop

Screenshot 2023-11-03 at 10 43 04 PM

Mobile

Screenshot 2023-11-03 at 10 43 41 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 17:12
@Imran92 Imran92 self-assigned this Nov 3, 2023
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #7264 (f8e072c) into trunk (dc309f7) will not change coverage.
Report is 1 commits behind head on trunk.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7264   +/-   ##
=========================================
  Coverage     50.79%   50.79%           
  Complexity    11049    11049           
=========================================
  Files           611      611           
  Lines         46703    46703           
  Branches        404      404           
=========================================
  Hits          23724    23724           
  Misses        22652    22652           
  Partials        327      327           
Files Coverage Δ
.../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 711d3f6...f8e072c. Read the comment docs.

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 f8e072c 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.

@merkushin
Copy link
Member

Looks better and more consistent now, but styles are a bit different. I took the example from the issue with a quiz.
In the quiz notice the header is larger (font-size).
CleanShot 2023-11-07 at 18 34 52@2x
CleanShot 2023-11-07 at 18 34 44@2x

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 9, 2023

Hi Dmitry 👋 Thanks for taking a look

Looks better and more consistent now, but styles are a bit different. I took the example from the issue with a quiz.
In the quiz notice the header is larger (font-size).

Right, actually we don't have an exact design for this specific notice. For example, we don't have a small Title, like "Awaiting Grade", rather it's a long text. So making it h2 sized makes the font look a bit too big. So I didn't change the font size intentionally, as we don't have a style to follow exactly. This is how it looks in h2 size. If you think it looks alright, we can change it. LMKWYT
image
image
image
image

@donnapep donnapep modified the milestones: 4.20.0, 4.19.2 Nov 9, 2023
merkushin
merkushin previously approved these changes Nov 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 👍

Base automatically changed from fix/not-enrolled-notice-style-in-lm to trunk November 15, 2023 15:29
@Imran92 Imran92 dismissed merkushin’s stale review November 15, 2023 15:29

The base branch was changed.

@donnapep
Copy link
Collaborator

donnapep commented Nov 15, 2023

I'm not sure why (as I don't have any template customizations and I don't appear to have set any global styles either), but this is what I'm seeing for this PR:

Screenshot 2023-11-15 at 2 38 18 PM

And when I'm logged out and on the trunk branch to test #7263, I see the following. The button text is the same color as the background and the "Please register" text is too big (because it's an h2, more below):

Screenshot 2023-11-15 at 2 43 58 PM

Could I have missed some Course theme changes or anything like that? 🤔

It's coming from this CSS, which is changing the button text color:

Screenshot 2023-11-15 at 3 06 37 PM

At some point, we seem to have changed the "Please register..." text to an h2. I'm guessing this was for styling purposes? The problem is that this is not semantic HTML as this text is not a heading. Can we change it back to a <p> and fix any styling with CSS? Incidentally, I think it looks just fine when I switch it to a <p>:

Screenshot 2023-11-15 at 3 03 02 PM

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 16, 2023

Hi @donnapep 👋 Thanks for taking a look!

I'm not sure why (as I don't have any template customizations and I don't appear to have set any global styles either), but this is what I'm seeing for this PR:

I've fixed it here 55ab34f!

Can we change it back to a

and fix any styling with CSS? Incidentally, I think it looks just fine when I switch it to a

:

Thanks for identifying the semantics issue, I've updated it here 6236726. The style looks go to me too, so I haven't changed anything else.

Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

👍🏻

@Imran92 Imran92 merged commit b4249f4 into trunk Nov 16, 2023
24 checks passed
@Imran92 Imran92 deleted the fix/not-logged-in-style-in-lm branch November 16, 2023 15:24
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 logged in
3 participants