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

Style the "Awaiting Grade" notice #7197

Merged
merged 46 commits into from
Nov 1, 2023
Merged

Conversation

donnapep
Copy link
Collaborator

@donnapep donnapep commented Sep 29, 2023

Resolves #7074.

Proposed Changes

  • Updates the styles for the "Awaiting Grade" notice on the quiz page.
  • Although the scope was only for the notice on the quiz page, I also updated just the colors of the "Awaiting Grade" notice on the lesson page. Since I broke it with my changes, the easiest fix was to match the colors. Matching the fonts would also be nice for consistency's sake, but that is outside the scope of this PR.
  • Unfortunately, the HTML of the Pending Teacher Grade and the Restart Quiz link in the notice and in the quiz footer are different. We probably should have synced up better on this one. Perhaps one day soon we'll be able to match the markup and use common styles. 🙏🏻
  • Made a change to ensure the third-party theme's learning-mode.css file loads after the generic Sensei one. Now we won't have to make theme CSS selectors more specific than the default selectors in order to override them. Instead, we just need to match them, and the styles loaded last (i.e. the theme's styles) win.

Testing Instructions

  1. Activate Divi or Astra.
  2. Add a quiz to a lesson and ensure that quiz is not set to auto-grade.
  3. Take the course as a student.
  4. View the "Awaiting grade" notice on the quiz page and ensure it matches the design.
  5. Verify that the colors of the "Awaiting grade" notice on the lesson page matches that of the quiz page (it's expected that the fonts won't match).
  6. Test on mobile as well.
  7. Check out this branch - Style the "Awaiting Grade" notice themes#7401.
  8. Activate the Course theme.
  9. Repeat steps 4 & 5 for all style variations.
  10. Disable the Quiz Graded email and view the notice again.
  11. Verify that "You’ll receive an email once it’s ready to view." is no longer part of the message.
  12. Bonus: Disable the email_customization feature flag and repeat step 11.

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

This change is specifically to load the third-party theme's `learning-mode.css` file after the generic Sensei one.

Now we won't have to make theme CSS selectors more specific than the default selectors in order to override them. Instead, we just need to match them, and the styles loaded last (i.e. the theme's styles) will win.
@donnapep donnapep added this to the 4.17.1 milestone Sep 29, 2023
@donnapep donnapep self-assigned this Sep 29, 2023
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #7197 (9fa8152) into trunk (b58d51a) will decrease coverage by 0.01%.
Report is 1 commits behind head on trunk.
The diff coverage is 44.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7197      +/-   ##
============================================
- Coverage     50.46%   50.46%   -0.01%     
- Complexity    10923    10931       +8     
============================================
  Files           605      605              
  Lines         46027    46062      +35     
  Branches        402      402              
============================================
+ Hits          23228    23244      +16     
- Misses        22472    22491      +19     
  Partials        327      327              
Files Coverage Δ
includes/class-sensei-quiz.php 69.48% <100.00%> (+0.51%) ⬆️
...ncludes/course-theme/class-sensei-course-theme.php 18.78% <0.00%> (ø)
...es/course-theme/class-sensei-course-theme-quiz.php 0.00% <0.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 67e35a9...9fa8152. Read the comment docs.

@donnapep donnapep marked this pull request as ready for review October 3, 2023 12:20
@donnapep donnapep requested a review from a team October 3, 2023 12:27
Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

Looks nice to me 👍 I've found a few small issues, not all of them are for addressing, as some looks like design inconsistencies to me-

  1. The text inside the notice is in two lines in the desktop version. I don't think it's important, but just mentioning as I've noticed this. Also looks like it's inconsistent among the variations
    Current:
image

Design:
image

  1. The notice in the mobile view is full-width in the design, but currently it has lateral spacings
    Current:
image

Design:
image

  1. The notice has a 4px border-radius in the design
    Current:
image

Design:
image

  1. In the mobile view, the design has only 24px spacing below the restart button, in our case, the 24px gets added up with the button spacing and adds more space below the button text
    Current:
Screenshot 2023-10-04 at 6 40 46 PM

Design:
image

@donnapep
Copy link
Collaborator Author

donnapep commented Oct 4, 2023

The text inside the notice is in two lines in the desktop version. I don't think it's important, but just mentioning as I've noticed this. Also looks like it's inconsistent among the variations

Yup, this one felt like a design inconsistency. I opted not to use a line break because none of the mobile views have it, and it didn't feel like a good use of my time considering the very low impact.

The notice in the mobile view is full-width in the design, but currently it has lateral spacings

Fixed in 56a373a. I think we should keep the button width consistent with what you've done in the footer, which is not full-width. WDYT?

The notice has a 4px border-radius in the design

Fixed here for just the Course theme - dbbdbdc

In the mobile view, the design has only 24px spacing below the restart button, in our case, the 24px gets added up with the button spacing and adds more space below the button text

Fixed here - 6aa4ba7

@Imran92

Imran92
Imran92 previously approved these changes Oct 5, 2023
Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

I think we should keep the button width consistent with what you've done in the footer, which is not full-width. WDYT?

Yap! it looks good to me too.

As we are rendering a button here and changing text based on different conditions, should we also add some tests to make sure they're rendering as expected and only when they should? But maybe we can also add the tests with the "Graded" notice when we'll have the full UI ready for all the states and conditions. I'm good with both.

I've also noticed that the "Pending teacher grade" button is being rendered in the graded quiz notice too while it should render only for quizzes awaiting grade. I'm good with that being handled in the "Graded" notice PR, but only a bit concerned as we're merging this PR to trunk and trunk can go to release, in which case it may result in a UI bug. It won't be an issue if we also merge the Graded notice PR before the next release.

Screenshot 2023-10-05 at 6 25 54 PM

I'm approving the PR as it looks great otherwise and in case we decide to take care of the concerns in another PR 👍 🚀

@donnapep
Copy link
Collaborator Author

donnapep commented Oct 5, 2023

I've also noticed that the "Pending teacher grade" button is being rendered in the graded quiz notice too while it should render only for quizzes awaiting grade.

Yes, I noticed this as well. My plan is not to merge this PR until the graded quiz notice is ready too. I think I'll add tests with that PR as well.

@donnapep donnapep added this to the 4.19.0 milestone Oct 11, 2023
@donnapep
Copy link
Collaborator Author

donnapep commented Oct 16, 2023

@Imran92

As we are rendering a button here and changing text based on different conditions, should we also add some tests to make sure they're rendering as expected and only when they should?

I don't think we should be writing unit tests to test UI. We should use e2e tests for that. However, we currently don't have any e2e tests for quizzes 😅 , so I think that bit may need to be part of a larger initiative. I did write some unit tests in #7210 for a function that you originally wrote and I refactored.

@donnapep donnapep modified the milestones: 4.19.0, 4.20.0 Oct 31, 2023
Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I've faced some issues when we have the latest Gutenberg enabled. The theme styles weren't getting applied as expected.

Without Gutenberg on

Screenshot 2023-11-01 at 2 52 42 PM

With Gutenberg on

Screenshot 2023-11-01 at 2 52 13 PM

I'll go ahead and approve it. We can address the issue with Gutenberg either here or separately

@donnapep
Copy link
Collaborator Author

donnapep commented Nov 1, 2023

I'll go ahead and approve it. We can address the issue with Gutenberg either here or separately

Separately I think, given the notices look pretty bad in Production currently. 🙂

@donnapep donnapep merged commit 423c951 into trunk Nov 1, 2023
21 of 23 checks passed
@donnapep donnapep deleted the update/awaiting-grade-notice branch November 1, 2023 11:32
@donnapep donnapep modified the milestones: 4.20.0, 4.19.0 Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update "Awaiting Grade" notice in Learning Mode
3 participants