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

Improve styles for sensei login page #7204

Merged
merged 15 commits into from
Oct 12, 2023
Merged

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Oct 5, 2023

Resolves #6368

Proposed Changes

  • Improved the style of the login page following the Woocommerce login
  • Improved the alert message shown when there's a login error
  • Fixed the issue of "My Messages" button getting rendered even for logged-out users

Testing Instructions

  1. Log out
  2. Go to My Courses page from frontend
  3. It should look better than before and nothing should be broken
  4. Try with all variations of the Course theme, Divi and Astra
  5. Enable registration from Settings -> General -> Anyone can Register
  6. Make sure it looks better with the Registration form too
  7. Check in mobile view as well for all the above
  8. Check the error alert with a wrong password

Course theme:

Screenshot 2023-10-05 at 4 32 17 PM Screenshot 2023-10-05 at 4 32 37 PM Screenshot 2023-10-05 at 4 32 52 PM Screenshot 2023-10-05 at 4 33 09 PM

Divi

Screenshot 2023-10-05 at 4 34 43 PM

Astra

Screenshot 2023-10-05 at 4 34 07 PM

Mobile

Screenshot 2023-10-05 at 4 35 34 PM

Error message

image (2)

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.17.1 milestone Oct 5, 2023
@Imran92 Imran92 requested a review from a team October 5, 2023 10:39
@Imran92 Imran92 self-assigned this Oct 5, 2023
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #7204 (c8a3e27) into trunk (ac5e9a4) will decrease coverage by 0.01%.
Report is 1 commits behind head on trunk.
The diff coverage is 14.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7204      +/-   ##
============================================
- Coverage     50.31%   50.31%   -0.01%     
- Complexity    10860    10862       +2     
============================================
  Files           601      601              
  Lines         45673    45675       +2     
  Branches        402      402              
============================================
  Hits          22980    22980              
- Misses        22366    22368       +2     
  Partials        327      327              
Files Coverage Δ
...cks/class-sensei-learner-messages-button-block.php 0.00% <0.00%> (ø)
includes/class-sensei-frontend.php 17.46% <16.66%> (-0.06%) ⬇️

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 7a5caf4...c8a3e27. Read the comment docs.

@@ -39,15 +39,15 @@

<label for="sensei_user_login"><?php esc_html_e( 'Username or Email', 'sensei-lms' ); ?> </label>

<input type="text" name="log" id="sensei_user_login" class="input" value="" size="20">
<input type="text" name="log" id="sensei_user_login" class="input input-text" value="" size="20">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't name input-text as per the convention because the same class name is used in the registration form. It's also looks like a convention to use this class in form input text fields, also used in woocommerce forms.

@Imran92 Imran92 added the PHP Templates This change modifies one or more PHP Templates. label Oct 5, 2023
@renatho renatho mentioned this pull request Oct 9, 2023
14 tasks
@renatho
Copy link
Contributor

renatho commented Oct 10, 2023

It's getting waaay better! 🎉

But I noticed a few things:

Different pages

  • When I set up a new site, the default My Course page is using the "Course List" variation, and the new styles aren't being applied there.
  • We also have the legacy My Courses with the [sensei_user_courses] shortcode where the new styles aren't being applied. Do we want to also improve the login there?

Maybe we could just use another class instead of the wp-block-sensei-lms-learner-courses to make it work for all the cases? If yes, I'd suggest adding a new class where we have the my-courses ID in sensei_login_form, just to not use the ID to encapsulate styles.

For discussion: Not part of this issue, but should we deprecate the Student Courses block? Or are we still using or encouraging it somehow?

Other styles

Changing the class manually to test the [sensei_user_courses] shortcode, I noticed it has a width: 90% on the inputs coming from assets/css/frontend.scss (see following screenshot), which also adds some styles to the login/registration form 🤔. Should we just tweak this? Or maybe try to remove those styles completely? I didn't investigate to see what is the reason for the 2 different files. Any idea?

Screenshot 2023-10-10 at 14 39 42

Error in the wrong place in some themes

With Divi and Twenty Twenty-One, I noticed that the error is displayed in the wrong place. See following screenshots.

Screenshot 2023-10-10 at 14 46 41

In Divi, I could see it after scrolling, otherwise the error is displayed behind the header

Screenshot 2023-10-10 at 15 15 18

Smaller screens

In Twenty Twenty-One, Two and Three, I noticed that it doesn't have enough space, so the "Lost your password?" is breaking the line in a weird way. Maybe we could improve in a way that it breaks the line in the link space only?

Screenshot 2023-10-10 at 14 43 47

@Imran92
Copy link
Contributor Author

Imran92 commented Oct 11, 2023

Hi @renatho , thanks so much for reviewing -

When I set up a new site, the default My Course page is using the "Course List" variation, and the new styles aren't being applied there.
We also have the legacy My Courses with the [sensei_user_courses] shortcode where the new styles aren't being applied. Do we want to also improve the login there?
Maybe we could just use another class instead of the wp-block-sensei-lms-learner-courses to make it work for all the cases? If yes, I'd suggest adding a new class where we have the my-courses ID in sensei_login_form, just to not use the ID to encapsulate styles.

I've just fallen back to use the previous my-courses ID selector as it is already there. It has solved issues with both the login shortcode and course list case e260a0d .

I noticed it has a width: 90% on the inputs coming from assets/css/frontend.scss

Yap right, thanks. It was some now-stale CSS. I've removed it here 22bdc7a

In Twenty Twenty-One, Two and Three, I noticed that it doesn't have enough space, so the "Lost your password?" is breaking the line in a weird way. Maybe we could improve in a way that it breaks the line in the link space only?

Actually, in all the TwentyTwenty series themes, the whole content was getting shrunk for some styles in those themes making the whole page look bad. So I've fixed it here e260a0d and now it has the same width as the woocommerce login page.

With Divi and Twenty Twenty-One, I noticed that the error is displayed in the wrong place. See following screenshots.

I did a bit of investigation and it turns out that the issue was not only with Divi and Twenty Twenty One, rather it was in all non-block themes. The notices got printed on top of the page, even above the navbar or header. It was due to how we handle the notices in the template. I've added a fix 7f7270d which makes the notices load nicely in all themes now.

Divi:
image

Twenty Twenty One:
image

Thanks!

@Imran92 Imran92 requested a review from renatho October 11, 2023 10:06
@renatho
Copy link
Contributor

renatho commented Oct 11, 2023

Thank you for the fixes, @Imran92!

I did a bit of investigation and it turns out that the issue was not only with Divi and Twenty Twenty One, rather it was in all non-block themes. The notices got printed on top of the page, even above the navbar or header. It was due to how we handle the notices in the template. I've added a fix 7f7270d which makes the notices load nicely in all themes now.

It seems it only fixes it for this specific notice, but the others using this method continue with the issue. I found #6173. If it's not very easy to fix, maybe we could revert the commit and work on that in another PR?


About Twenty Twenty-One, I still noticed that we don't have spacing on the buttons. But feel free to ignore this one if it's not easy or needs a hacky solution to be fixed, since it doesn't seem to be a theme very used for Sensei currently.

Screenshot 2023-10-11 at 10 49 52

@Imran92
Copy link
Contributor Author

Imran92 commented Oct 11, 2023

It seems it only fixes it for this specific notice, but the others using this method continue with the issue. I found #6173. If it's not very easy to fix, maybe we could revert the commit and work on that in another PR?

I think we need to address the other notice issues in a separate PR as this PR is login-specific and there are good chances of side effects of any global changes.

About Twenty Twenty-One, I still noticed that we don't have spacing on the buttons. But feel free to ignore this one if it's not easy or needs a hacky solution to be fixed, since it doesn't seem to be a theme very used for Sensei currently.

Right, I also don't think we probably shouldn't be too concerned about twenty twenty one theme issues. But I just added a little style here to add some space there only for twenty twenty one theme c8a3e27

Copy link
Contributor

@renatho renatho 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!! =)

I think we need to address the other notice issues in a separate PR as this PR is login-specific and there are good chances of side effects of any global changes.

About this, I wonder if we should revert the commit with the fix we did specifically to this part. So when we fix it globally, it gets fixed the same way in all the places. WDYT?

@donnapep donnapep removed this from the 4.18.0 milestone Oct 11, 2023
@donnapep donnapep added this to the 4.19.0 milestone Oct 11, 2023
@Imran92
Copy link
Contributor Author

Imran92 commented Oct 12, 2023

About this, I wonder if we should revert the commit with the fix we did specifically to this part. So when we fix it globally, it gets fixed the same way in all the places. WDYT?

That's a very good thought @renatho and I also thought about this. But the login form has its own function call to notice printing to make sure it is displayed immediately above the login form or login block

<?php Sensei()->notices->maybe_print_notices(); ?>
which is different from other pages in general, separated from the global way of displaying notices. There's a chance that we'll still need to keep this change even then depending on how we decide to fix that. So for now, I'm keeping the current change to make it look not broken.

@Imran92 Imran92 merged commit d0c27dc into trunk Oct 12, 2023
22 of 24 checks passed
@Imran92 Imran92 deleted the add/styles-for-sensei-login-page branch October 12, 2023 08:46
@m1r0 m1r0 modified the milestones: 4.19.0, 4.18.0 Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP Templates This change modifies one or more PHP Templates. [Project] Frontend Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Style login page
4 participants