-
Notifications
You must be signed in to change notification settings - Fork 198
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 stylings for footer in awaiting grade quizzes in learning mode #7190
Update stylings for footer in awaiting grade quizzes in learning mode #7190
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #7190 +/- ##
============================================
+ Coverage 49.83% 50.03% +0.19%
- Complexity 10684 10698 +14
============================================
Files 601 601
Lines 45160 45182 +22
Branches 402 402
============================================
+ Hits 22504 22605 +101
+ Misses 22329 22250 -79
Partials 327 327
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
$is_learning_mode = \Sensei_Course_Theme_Option::has_learning_mode_enabled( $course_id ); | ||
$is_awaiting_grade = false; | ||
|
||
$lesson_status = \Sensei_Utils::user_lesson_status( $lesson_id, get_current_user_id() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Psalm is marking an issue here. Could we fix the PHPDoc return of user_lesson_status
to also have an array of WP_Comments
, which seems to be another possible return?
$is_learning_mode = \Sensei_Course_Theme_Option::has_learning_mode_enabled( $course_id ); | ||
$is_awaiting_grade = false; | ||
|
||
$lesson_status = \Sensei_Utils::user_lesson_status( $lesson_id, get_current_user_id() ); | ||
|
||
if ( $lesson_status ) { | ||
$lesson_status = is_array( $lesson_status ) ? $lesson_status[0] : $lesson_status; | ||
|
||
$is_awaiting_grade = 'ungraded' === $lesson_status->comment_approved; | ||
} | ||
|
||
if ( $is_learning_mode && $is_awaiting_grade && 'quiz' === get_post_type() ) { | ||
return ''; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT to extract these lines to a private method returning a boolean? This render
method seems to be a method growing too much. 🤭
includes/class-sensei-quiz.php
Outdated
$is_learning_mode = Sensei_Course_Theme_Option::has_learning_mode_enabled( $course_id ); | ||
$is_awaiting_grade = false; | ||
|
||
$lesson_status = \Sensei_Utils::user_lesson_status( $lesson_id, get_current_user_id() ); | ||
|
||
if ( $lesson_status ) { | ||
$lesson_status = is_array( $lesson_status ) ? $lesson_status[0] : $lesson_status; | ||
|
||
$is_awaiting_grade = 'ungraded' === $lesson_status->comment_approved; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the previous comment, maybe we could have that method in this class being used in both places to avoid duplication? WDYT?
Feel free to disagree since I'm re-learning about Sensei and probably missing a lot of context. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well!
I just noticed one last thing, but I'm not sure if it's expected, so I'm already approving it. 😉 In Divi and Twenty Twenty-Three themes there is an extra spacing around the buttons.
It seems the same doesn't happen for the questions because of this class with the max-width
:
Good catch @renatho ! Thank you! |
Resolves #7101
Proposed Changes
Testing Instructions
In the design, the variations of Course theme have 0.6 opacity, but the default variation has 0.4, so I only set the 0.4, and didn't add 0.6 for the variations because it'd require adding CSS as string in all the JSON files of the variations which is not very manageable. 0.2 difference in opacity hopefully shouldn't feel too different in the variations.
Desktop:
Desktop - without retakes:
Mobile:
Pre-Merge Checklist