fix: ensure that lost modal styles are applied throughout application #4291
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request updates multiple Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/frontend-pwa/src/components/participant/LeaveLeaderboardModal.tsx (1)
39-39: Approved: Modal style improvementsThe changes to the
Modalcomponent'sclassNameprop improve the layout consistency:
- Adding
min-h-maxensures the modal always accommodates its content height.- Changing
pt-0to!pt-0guarantees zero top padding, overriding any conflicting styles.These adjustments should resolve any layout issues related to the modal's height and padding.
For better readability and maintainability, consider splitting the
classNameobject into multiple lines:className={{ content: 'h-max min-h-max w-[40rem] self-center !pt-0', }}This format makes it easier to add or modify classes in the future.
apps/frontend-manage/src/components/sessions/cockpit/SessionQRModal.tsx (2)
46-46: LGTM! Consider adding a comment for clarity.The addition of
!w-maxto the Modal's content className is appropriate. It ensures the modal takes up the maximum width needed for its content, which aligns with the PR objective of applying lost modal styles.For improved clarity, consider adding a brief comment explaining the purpose of
!w-max. For example:content: 'h-max max-h-full !w-max max-w-6xl overflow-y-auto', +// !w-max ensures the modal takes up the maximum width needed for its contentThis comment would help future developers understand the intent behind this specific style.
Line range hint
1-95: Consider extracting QR code rendering logicThe component is well-structured and follows React best practices. To further improve code organization and reusability, consider extracting the QR code rendering logic into a separate component.
You could create a new component, for example,
QRCodeWithLink, that encapsulates the QR code, title, description, and "Present QR Code" button. This would reduce duplication and make theSessionQRModalcomponent more concise. Here's a sketch of how it could look:interface QRCodeWithLinkProps { title: string; description: string; path: string; buttonCy: string; } function QRCodeWithLink({ title, description, path, buttonCy }: QRCodeWithLinkProps) { const t = useTranslations() return ( <div className="flex-1"> <H3>{title}</H3> <Prose>{description}</Prose> <QR className={{ title: 'text-base', canvas: 'flex justify-center' }} path={path} width={100} /> <Link passHref href={`/qr${path}`} target="_blank"> <Button fluid className={{ root: twMerge( 'bg-primary-80 mt-2 h-9 text-lg font-bold text-white' ), }} data={{ cy: buttonCy }} > <Button.Label>{t('manage.general.presentQrCode')}</Button.Label> </Button> </Link> </div> ) }Then, in the
SessionQRModal, you could use it like this:<div className="flex flex-col gap-8 md:flex-row"> <QRCodeWithLink title="Account Link" description={t('manage.cockpit.qrCodeAccountLinkDescription')} path={accountRelativeLink} buttonCy={`qr-link-${shortname}`} /> <QRCodeWithLink title={t('manage.cockpit.qrCodeDirectLinkTitle')} description={t('manage.cockpit.qrCodeDirectLinkDescription')} path={sessionRelativeLink} buttonCy={`qr-direct-link-${sessionId}`} /> </div>This refactoring would make the code more maintainable and easier to update in the future.
apps/frontend-manage/src/components/sessions/EmbeddingModal.tsx (3)
37-50: Improved link styling and layoutThe changes to the
LazyHMACLinkcomponent enhance the layout and styling of the link display. Themax-w-fullclass on the outer<div>prevents overflow, while the newclassNameon the<a>tag improves text wrapping and width constraints. These modifications align well with the PR objective of ensuring consistent modal styles throughout the application.Consider adding a
titleattribute to the<a>tag with the full link URL. This would provide a tooltip with the complete link when users hover over it, which can be helpful when the link is truncated due to themax-w-[calc(100%-3.5rem)]constraint.<a data-cy={`open-embedding-link-session-${sessionId}`} className="max-w-[calc(100%-3.5rem)] break-words text-sm" + title={link} > {link} </a>
Line range hint
107-109: Enhanced link parameters for better context and controlThe updates to the
LazyHMACLinkcomponent call within the questions mapping improve the functionality and flexibility of the embedded links. The addition ofquestionIxandshowSolutionparameters provides more context and control over the embedded view, whilehideControls=trueensures a cleaner presentation.For consistency, consider applying the
hideControls=trueparameter to the evaluation link as well (line 95). This would ensure a uniform appearance across all embedded views.<LazyHMACLink sessionId={sessionId} params={``} />to
<LazyHMACLink sessionId={sessionId} params={`hideControls=true`} />
Line range hint
114-117: Added leaderboard link with consistent stylingThe addition of a
LazyHMACLinkcomponent for the leaderboard enhances the functionality of the modal. The use ofleaderboard=trueandhideControls=trueparameters maintains consistency with the other embedded links.To improve clarity, consider adding a comment explaining the purpose of the leaderboard link, especially if it's a new feature. This would help other developers understand its significance quickly.
<div> + {/* Leaderboard link for displaying user rankings */} <div className="w-30 font-bold"> {t('shared.generic.leaderboard')}: </div> <LazyHMACLink sessionId={sessionId} params={`leaderboard=true&hideControls=true`} /> </div>apps/frontend-manage/src/components/questions/Question.tsx (1)
Line range hint
243-288: LGTM! Consider adding error handling.The changes to the Modal component for question deletion look good. The async function in
onPrimaryActioncorrectly handles the deletion process, including updating the Apollo cache and the UI.Consider adding error handling to the delete operation. For example:
try { await deleteQuestion({ // ... existing code ... }); unsetDeletedQuestion(id); setIsDeletionModalOpen(false); } catch (error) { console.error('Error deleting question:', error); // Optionally, show an error message to the user }This will ensure that any errors during the deletion process are caught and logged, improving the robustness of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/frontend-manage/src/components/courses/modals/EnableGamificationModal.tsx (0 hunks)
- apps/frontend-manage/src/components/questions/Question.tsx (2 hunks)
- apps/frontend-manage/src/components/sessions/EmbeddingModal.tsx (1 hunks)
- apps/frontend-manage/src/components/sessions/cockpit/SessionQRModal.tsx (1 hunks)
- apps/frontend-manage/src/components/sessions/creation/liveQuiz/AdvancedLiveQuizSettings.tsx (2 hunks)
- apps/frontend-pwa/src/components/participant/LeaveLeaderboardModal.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- apps/frontend-manage/src/components/courses/modals/EnableGamificationModal.tsx
🔇 Additional comments (5)
apps/frontend-manage/src/components/sessions/EmbeddingModal.tsx (1)
Line range hint
1-124: Overall assessment: Improved modal functionality and consistencyThe changes in this file successfully address the PR objective of ensuring that modal styles are applied throughout the application. The modifications to the
LazyHMACLinkcomponent and its usage withinEmbeddingModalenhance the layout, styling, and functionality of the embedded links. The addition of the leaderboard link further improves the modal's features.Key improvements:
- Better link styling and layout in
LazyHMACLink- Enhanced link parameters for questions, providing more context and control
- Addition of a leaderboard link with consistent styling
The suggested minor improvements (adding a title attribute, ensuring consistency with
hideControls, and adding a comment for the leaderboard link) would further enhance the code's clarity and consistency.apps/frontend-manage/src/components/sessions/creation/liveQuiz/AdvancedLiveQuizSettings.tsx (4)
49-49: Improved modal responsiveness and layoutThe updated
classNameprop enhances the modal's responsiveness and layout:
!w-fullensures the modal takes full width on smaller screens.max-w-[60rem]prevents the modal from becoming too wide on larger screens.!pb-0removes bottom padding, allowing for custom padding within the modal content.These changes provide better control over the modal's appearance across different screen sizes.
52-52: Enhanced responsive layoutThe updated outer div's
classNameimproves the component's responsiveness:
- Vertical layout on smaller screens with
flex flex-col.- Horizontal layout on medium and larger screens with
md:flex-row.- Appropriate spacing with
gap-6for vertical layout andmd:gap-0for horizontal layout.This change ensures better content organization and readability across different device sizes.
53-53: Improved inner layout responsivenessThe updated inner divs'
classNameproperties enhance the layout's flexibility:
- Both divs use
w-fullfor full width on smaller screens.- They switch to
md:w-1/2on medium screens and above, creating a two-column layout.- The first div adds
md:mr-8for right margin on larger screens, ensuring proper spacing.These changes create a more adaptable and visually balanced layout across various screen sizes.
Also applies to: 79-79
Line range hint
49-79: Summary: Effective implementation of responsive designThe changes in this PR successfully address the objective of ensuring modal styles are applied throughout the application. The modifications to the
AdvancedLiveQuizSettingscomponent significantly improve its responsiveness and layout:
- The modal now has a more controlled width and padding.
- The outer layout adapts between vertical and horizontal arrangements based on screen size.
- Inner elements adjust their width and spacing to maintain visual balance across devices.
These improvements enhance the overall user experience without altering the component's core functionality. The implementation follows responsive design best practices and should integrate well with the rest of the application.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
modal-styles
|
| Run status |
|
| Run duration | 09m 43s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 09m 33s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
47
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
@uzh-bf/design-systemacross multiple components for enhanced functionality and consistency.