-
Notifications
You must be signed in to change notification settings - Fork 297
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
Quiz exercises
: Adjust user interface of quiz header and footer
#9744
base: develop
Are you sure you want to change the base?
Quiz exercises
: Adjust user interface of quiz header and footer
#9744
Conversation
|
WalkthroughThe pull request introduces significant updates to the quiz participation interface, enhancing both the HTML structure and SCSS styling. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/shared/connection-status/connection-status.component.html (1)
8-12
: Consider enhancing accessibility for connection statusWhile the implementation is correct, consider adding
aria-live="polite"
to the badge div to ensure screen readers announce connection status changes.- <div class="badge bg-danger connectedExam"> + <div class="badge bg-danger connectedExam" aria-live="polite">src/main/webapp/app/exercises/quiz/participate/quiz-participation.component.scss (2)
1-21
: LGTM! Consider adding ARIA attributes for time warnings.The styling for time indicators and score display is well-structured and follows theming guidelines. The use of CSS variables ensures consistent theming across light and dark modes.
Consider adding corresponding ARIA attributes in the HTML template when these warning classes are applied to ensure screen readers can announce time-critical states:
<div class="time-warning" role="alert" aria-live="polite"> <!-- time warning content --> </div>
Line range hint
1-163
: LGTM! Well-structured SCSS with clear separation of concerns.The file maintains a clear organization with:
- Distinct sections for different UI components
- Consistent use of CSS variables for theming
- Responsive design considerations
- Proper nesting of selectors
Consider splitting this file into smaller, focused modules if it grows larger:
_quiz-overlays.scss
_quiz-indicators.scss
_quiz-layout.scss
src/main/webapp/app/exercises/quiz/participate/quiz-participation.component.html (1)
34-35
: Simplify overlapping conditions in 'ngClass' for time indicatorsThe conditions for applying the
time-critical
andtime-warning
classes overlap, which may lead to both classes being applied simultaneously whenremainingTimeSeconds
is less than 60 or less thanquizExercise.duration! / 4
. To ensure only one class is applied at a time, consider adjusting the conditions to make them mutually exclusive.Apply this diff to adjust the conditions:
[ngClass]="{ - 'time-critical': remainingTimeSeconds < 60 || remainingTimeSeconds < quizExercise.duration! / 4, - 'time-warning': remainingTimeSeconds < 120 || remainingTimeSeconds < quizExercise.duration! / 2, + 'time-critical': remainingTimeSeconds < 60 || remainingTimeSeconds < quizExercise.duration! / 4, + 'time-warning': (remainingTimeSeconds >= 60 && remainingTimeSeconds < 120) || (remainingTimeSeconds >= quizExercise.duration! / 4 && remainingTimeSeconds < quizExercise.duration! / 2), }"This change ensures that the
time-warning
class is only applied whenremainingTimeSeconds
is between 60 and 120 seconds or between one-quarter and half of thequizExercise.duration
, and thetime-critical
class is applied whenremainingTimeSeconds
is less than 60 seconds or less than one-quarter of thequizExercise.duration
.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/main/webapp/app/exercises/quiz/participate/quiz-participation.component.html
(2 hunks)src/main/webapp/app/exercises/quiz/participate/quiz-participation.component.scss
(2 hunks)src/main/webapp/app/shared/connection-status/connection-status.component.html
(1 hunks)src/main/webapp/app/shared/connection-status/connection-status.component.scss
(2 hunks)src/main/webapp/i18n/de/quizExercise.json
(1 hunks)src/main/webapp/i18n/en/quizExercise.json
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/webapp/app/shared/connection-status/connection-status.component.scss
- src/main/webapp/i18n/de/quizExercise.json
- src/main/webapp/i18n/en/quizExercise.json
🧰 Additional context used
🔇 Additional comments (3)
src/main/webapp/app/shared/connection-status/connection-status.component.html (3)
1-7
: LGTM! Proper usage of new Angular control flow syntax
The code correctly uses the new @if
syntax instead of *ngIf
, following the latest Angular guidelines. The badge and icon styling are well-structured and consistent.
Line range hint 13-13
: LGTM! Proper use of content projection
The named content projection with select="[innerContent]"
follows Angular best practices and allows for flexible content insertion.
Line range hint 1-14
: Verify integration with quiz participation component
Let's ensure this component integrates properly with the quiz participation component where it's being used.
.sticky-top-stepwizard { | ||
position: sticky; | ||
top: 46px; | ||
z-index: 18; | ||
} | ||
|
||
.sticky-bottom-quiz-footer { | ||
position: sticky; | ||
bottom: -1rem; // Removes margin from 'course-body' class to improve layout alignment | ||
z-index: 18; | ||
} |
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.
💡 Codebase verification
Establish a standardized z-index scale to prevent layering conflicts.
- Multiple z-index values found (1, 2, 3, 10, 12, 18, 31). Create a shared SCSS variables file to maintain consistency and avoid overlaps.
🔗 Analysis chain
Document z-index layering and verify sticky positioning across viewports.
The sticky positioning implementation looks good, but there are a few considerations:
- The z-index values should be documented to prevent future layering conflicts
- The negative bottom margin workaround might need verification across different viewports
Let's check for potential z-index conflicts and related sticky elements:
Consider creating a z-index scale in a shared SCSS variables file:
// _z-index.scss
$z-index-quiz-sticky: 18;
$z-index-quiz-overlay: 31;
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other z-index declarations and sticky positioning that might conflict
# Check for other z-index declarations
rg "z-index:" src/main/webapp/app/exercises/quiz/
# Check for other sticky positioned elements
rg "position:\s*sticky" src/main/webapp/app/exercises/quiz/
Length of output: 2453
|
|
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.
[Tested on TS6]
Works just as described it should. Look good to me
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.
Tested on TS 6, Reapprove.
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.
Tested on TS3, reapprove
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.
Reapprove
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.
Thanks for the introduction of the templates and re-structuring which makes the template easier to read
I left some minor comments where I do not have a strong opinion, I see more work for this component but not within the scope of this PR:
- I think that it might be good to divide the
quiz-participation.component.html
into further smaller components (e.g. another on vor the Overlay section and one for the Step Wizard) - The complexity of the template could also additionally be reduced by adding computed signals, which would also require the introduction of signals 😅
@@ -132,6 +133,7 @@ export class QuizParticipationComponent implements OnInit, OnDestroy { | |||
private quizExerciseService: QuizExerciseService, | |||
private participationService: ParticipationService, | |||
private route: ActivatedRoute, | |||
private router: Router, |
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.
Was it an active decision not use the more explicit inject here?
private readonly router = inject(Router);
Checklist
General
Client
Motivation and Context
The previous footer had issues, especially on smaller screens.
Description
Rearranged some information and buttons. Removed duplicated information (e.g. practice or preview). Stick footer to the bottom. Align the design in general and refactored some code.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Screenshots
Before:
Live:
Practice:
After:
Live:
<img width="1727" alt="Screenshot 2024-11-12 at 15 00 37" src="https://github.com/user-attachments/
assets/717221e5-35f9-4e5a-baef-47c9bd3faaf6">
Practice:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Localization