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

Update footer styles in multi page quiz #7268

Merged
merged 22 commits into from
Nov 15, 2023

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Nov 6, 2023

Resolves #7243

Proposed Changes

  • Updated styles for multi-paged quizzes in learning mode

Testing Instructions

  1. Add a multi-page quiz to a lesson.
  2. Select the Allow Retakes checkbox.
  3. Checkout this branch of Course theme Fix extra padding issue in outline buttons themes#7462
  4. As a student, view the quiz on the frontend.
  5. Scroll to the footer.
  6. Make sure the design matches the new structure of footers across LM
  7. Make sure normal single page footer is not broken
  8. Make sure lesson footer is not broken
  9. Check in mobile view
  10. Check for all variations of course theme and other top used themes

Course theme

Screenshot 2023-11-06 at 3 55 49 PM
Screenshot 2023-11-06 at 3 56 20 PM

Astra

Screenshot 2023-11-06 at 3 56 48 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.19.1 milestone Nov 6, 2023
@Imran92 Imran92 requested a review from a team November 6, 2023 09:58
@Imran92 Imran92 self-assigned this Nov 6, 2023
@Imran92 Imran92 changed the base branch from trunk to add/stylesheets-for-course-theme-variations November 6, 2023 09:59
@Imran92
Copy link
Contributor Author

Imran92 commented Nov 6, 2023

Having only "Next" as the button text does not look good to me for some reason, should we change this to "Next Questions" or "Next Page" or something like that? In the context of Lessons, we use "Next Lesson" as button text

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 1d08077 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.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #7268 (1d08077) into trunk (261f533) will not change coverage.
Report is 9 commits behind head on trunk.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #7268   +/-   ##
=========================================
  Coverage     50.74%   50.74%           
  Complexity    11051    11051           
=========================================
  Files           610      610           
  Lines         46649    46649           
  Branches        402      402           
=========================================
  Hits          23671    23671           
  Misses        22651    22651           
  Partials        327      327           
Files Coverage Δ
...ludes/blocks/course-theme/class-lesson-actions.php 98.00% <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 97f952a...1d08077. Read the comment docs.

@donnapep donnapep modified the milestones: 4.19.1, 4.20.0 Nov 6, 2023
@donnapep
Copy link
Collaborator

donnapep commented Nov 6, 2023

Having only "Next" as the button text does not look good to me for some reason, should we change this to "Next Questions" or "Next Page" or something like that? In the context of Lessons, we use "Next Lesson" as button text

If we use "Next Lesson" for lessons, then "Next Question" makes the most sense here. 👍🏻

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 7, 2023

If we use "Next Lesson" for lessons, then "Next Question" makes the most sense here. 👍🏻

Thank you, I've updated the Next and Previous buttons here 8a9a0e9, now they say "Previous Question" and "Next Question"

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 to me, in general. Tested it when the quiz was not submitted and when it was submitted: both looks good.

I found an edge case, which is a bit broken. I don't know if we want to address it here (or at all). For this reason I don't approve the PR, let me know if we're fine to merge it as is and I approve it.

Now, to the edge case. It happens when the student completed the lesson, then the teacher adds a quiz to that lesson. When I open the quiz as a student I see it like that (pagination links are smaller, the "Next Question" button is in a different place):
CleanShot 2023-11-07 at 16 59 11@2x

At the same time, we don't need pagination here: all questions are here.

@merkushin
Copy link
Member

And as for the "Next Question"/"Next Page" discussion.
It is possible to have more than one question per page, so "Next Question" (singular) looks not correct here, I think.
In the screenshot, you can see I am on the second page and I have two questions per page, but I see "Previous Question" on the button. I would expect "Previous Questions" in this case.
CleanShot 2023-11-07 at 17 12 10@2x

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 8, 2023

Hi @merkushin 👋 Thanks for taking a look.

I found an edge case, which is a bit broken. I don't know if we want to address it here (or at all). For this reason I don't approve the PR, let me know if we're fine to merge it as is and I approve it.

Now, to the edge case. It happens when the student completed the lesson, then the teacher adds a quiz to that lesson. When I open the quiz as a student I see it like that (pagination links are smaller, the "Next Question" button is in a different place):

I've investigated the issue to find the root cause, found the cause after deep investigation, I'm sharing it in details because I felt a bit concerned about it as it's happening due to some changes related to HPPS and you may need to look for and handle potential similar issues in other places as well if they exist.

In learning mode, when viewing a quiz, we use the lesson or the quiz template conditionally. If the quiz is not taken by the student, we load the quiz template, and if the quiz is in awaiting-grade or any later status, we use the lesson template. you can find the logic here.

The problem is, for HPPS, I see that a change was introduced here and many other places recently to read quiz/lesson status from https://github.com/Automattic/sensei/pull/7185/files quiz repository instead. Previously, when a quiz was not taken by a student but the lesson was marked as completed, the status we got in comment was 'completed', but quiz repository is returning "passed" in that case, which is not correct because the student did not actually pass the quiz. I reverted the repository change temporarily to check if it works as expected, and it did. So I think we should handle the status issue in the HPPS project. I think it probably is an important issue because it is impacting not only the multi page quizzes, but other normal quizzes too and apart from the LM template loading, it can have impact in other places as well.

I would expect "Previous Questions" in this case.

Updated here for Next and Previous Questions f71c93a 👍

@merkushin
Copy link
Member

Thanks for investigation @Imran92!
You're right, the template was determined wrongly because of those changes.

Just to inform I want to notice, that we can't return 'completed' status for a quiz progress as it breaks the logic ('completed' is not a quiz progress status, that's a lesson progress status) and will break compatibility between comments-based and tables-based versions.
However, if you notice a similar issue, please ping me or Miro for sooner resolution. I fixed this issue and a similar one in this PR (it's not open yet as I want to add some tests).

Also I addressed this issue there:

At the same time, we don't need pagination here: all questions are here.

It wasn't related to HPPS project, but highlights the problem of unified progress for a lesson and a quiz.
In fact we need pagination there, but also have to output questions paginated.

merkushin
merkushin previously approved these changes Nov 9, 2023
@donnapep donnapep modified the milestones: 4.20.0, 4.19.2 Nov 9, 2023
@Imran92
Copy link
Contributor Author

Imran92 commented Nov 9, 2023

Hi @merkushin 👋 Thanks for the review!

Just to inform I want to notice, that we can't return 'completed' status for a quiz progress as it breaks the logic ('completed' is not a quiz progress status, that's a lesson progress status) and will break compatibility between comments-based and tables-based versions.

Yap, fully agreed. The problem was it was returning "Passed" which is a quiz-specific one. We just needed it to be something other than these 4 quiz-specific ones [ 'ungraded', 'graded', 'passed', 'failed' ]. Thanks for handling it now!

However, if you notice a similar issue, please ping me or Miro for sooner resolution

Sure I will, thanks!

I'm not merging this PR yet as it depends on this PR #7256.

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

The base branch was changed.

@Imran92
Copy link
Contributor Author

Imran92 commented Nov 14, 2023

Hi @merkushin 👋 Sorry the review got dismissed after the base branch was merged to trunk, can you kindly do it again?

Thanks!!

@Imran92 Imran92 merged commit 93de5b8 into trunk Nov 15, 2023
24 checks passed
@Imran92 Imran92 deleted the fix/button-styles-for-multi-page-quiz branch November 15, 2023 01:51
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 footer of multi-page quiz in Learning Mode
3 participants