enhance(apps/frontend-manage): replace navigation on lecturer frontend #4469
Conversation
📝 WalkthroughWalkthroughThis pull request involves an update of the Changes
Possibly related PRs
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: 2
🧹 Nitpick comments (9)
apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizTimeline.tsx (1)
168-168: Consider consistent icon sizing across all buttons.While adjusting the icon size with
h-4makes sense with the reduced button height, this explicit sizing is only applied to one icon. Consider applying consistent sizing to all button icons in this component.- <FontAwesomeIcon icon={faUpRightFromSquare} className="h-4" /> + <FontAwesomeIcon icon={faUpRightFromSquare} />And update the Button.Icon component's default styling in the design system to handle icon sizing consistently.
packages/markdown/package.json (1)
43-43: Consider using a version range for peer dependencies.The strict version requirement
"3.0.0-alpha.45"for@uzh-bf/design-systemin peerDependencies might cause compatibility issues. Consider using a version range (e.g.,"^3.0.0-alpha.45") to allow minor updates.- "@uzh-bf/design-system": "3.0.0-alpha.45", + "@uzh-bf/design-system": "^3.0.0-alpha.45",apps/frontend-manage/src/components/common/Header.tsx (3)
30-30: Address the TODO: Generalize the query for the new catalogueThere's a TODO comment indicating that the
GetCollectionSharingRequestsDocumentquery needs to be generalized for the new catalogue. Do you need assistance with this task?Do you want me to help with generalizing this query or open a GitHub issue to track this task?
Line range hint
208-239: Avoid rendering empty dropdown when disabledCurrently, when the dropdown is disabled, the
MenubarTriggeris still rendered, which might confuse users as it appears interactive. To improve user experience, consider not rendering the dropdown at all when it's disabled.Apply this diff to conditionally render the dropdown:
<MenubarMenu> + {!disabled && ( <MenubarTrigger disabled={disabled} data-cy={data?.cy} data-test={data?.test} className={twMerge( 'text-base hover:cursor-pointer', hasIconAndLabel && 'flex flex-row items-center gap-2', !iconOnly && !disabled && dynamicUnderline, active && 'text-black after:scale-x-100', disabled && 'hover:cursor-not-allowed', className?.trigger )} > {/* Trigger content */} </MenubarTrigger> {!disabled ? ( <MenubarContent className={className?.content}> {/* Content */} </MenubarContent> ) : null} + )} </MenubarMenu>
Line range hint
208-239: Simplify conditional rendering of MenubarContentThe current conditional rendering of
MenubarContentuses a ternary operator that returnsnullwhen the dropdown is disabled. This can be simplified for better readability.Apply this diff to simplify the conditional rendering:
{!disabled ? ( <MenubarContent className={className?.content}> {/* Content */} </MenubarContent> - ) : null} + )}apps/frontend-manage/src/components/common/Navigation.tsx (4)
1-1: Address the TODO: Extract component to design systemThere's a TODO comment indicating that this
Navigationcomponent should be extracted to the design system with generalized props. Do you need assistance with this task?Do you want me to help with extracting this component to the design system or open a GitHub issue to track this task?
59-60: Simplify boolean expressionsThe boolean expressions used to determine
hasIconAndLabelandiconOnlycan be simplified for clarity.Apply this diff to simplify the expressions:
- const hasIconAndLabel = - typeof label !== 'undefined' && typeof icon !== 'undefined' - const iconOnly = typeof label === 'undefined' && typeof icon !== 'undefined' + const hasIconAndLabel = label && icon + const iconOnly = !label && icon
74-75: Improve disabled state stylingWhen the button is disabled, it still has the
hover:cursor-not-allowedclass, but thehoverpseudo-class is unnecessary as the cursor should always reflect the disabled state.Apply this diff to adjust the styling:
- disabled && 'hover:cursor-not-allowed', + disabled && 'cursor-not-allowed',
220-221: Consistent class names in NavigationDropdownIn
NavigationDropdown, the className for the submenu trigger usesclassName?.label, which might be confusing. Ensure that class names are consistently named and applied.Apply this diff to rename the className property:
<MenubarSubTrigger className={twMerge( 'h-8 text-base hover:cursor-pointer', - className?.label + element.className?.label )} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/auth/package.json(1 hunks)apps/backend-docker/package.json(1 hunks)apps/docs/package.json(1 hunks)apps/frontend-control/package.json(1 hunks)apps/frontend-manage/package.json(1 hunks)apps/frontend-manage/src/components/common/Header.tsx(2 hunks)apps/frontend-manage/src/components/common/Navigation.tsx(1 hunks)apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizTimeline.tsx(3 hunks)apps/frontend-pwa/package.json(1 hunks)apps/func-incoming-responses/package.json(1 hunks)apps/func-response-processor/package.json(1 hunks)apps/office-addin/package.json(1 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)packages/markdown/package.json(1 hunks)packages/shared-components/package.json(1 hunks)packages/transactional/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/shared-components/package.json
- apps/office-addin/package.json
- apps/frontend-manage/package.json
- apps/auth/package.json
- apps/backend-docker/package.json
- apps/frontend-pwa/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: SonarCloud
- GitHub Check: cypress-run
🔇 Additional comments (10)
apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizTimeline.tsx (2)
118-118: LGTM! Consistent button height adjustments.The height reduction from
h-10toh-8has been consistently applied across all buttons in the control panel section, maintaining UI uniformity.Also applies to: 146-146, 163-163, 182-182
Line range hint
176-190: LGTM! Well-implemented feedback quiz section.The feedback quiz conditional block is well-implemented with:
- Consistent button styling and height
- Proper use of twMerge for class composition
- Appropriate conditional rendering
packages/i18n/messages/en.ts (1)
2033-2036: LGTM! Analytics section enhanced with new categories.The new translation keys provide better categorization for analytics features. The translations are consistent with their German counterparts.
Let's verify that all new keys have corresponding translations in the German file:
✅ Verification successful
✓ All analytics keys are properly translated
All new analytics keys (
activity,performance,quizzes,olderCourses) have corresponding and semantically correct translations in the German language file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all new analytics keys exist in both language files # and have appropriate translations echo "Checking for new analytics keys in both language files..." # Check English keys rg "activity: '.*'" packages/i18n/messages/en.ts rg "performance: '.*'" packages/i18n/messages/en.ts rg "quizzes: '.*'" packages/i18n/messages/en.ts rg "olderCourses: '.*'" packages/i18n/messages/en.ts # Check German translations rg "activity: '.*'" packages/i18n/messages/de.ts rg "performance: '.*'" packages/i18n/messages/de.ts rg "quizzes: '.*'" packages/i18n/messages/de.ts rg "olderCourses: '.*'" packages/i18n/messages/de.tsLength of output: 979
packages/i18n/messages/de.ts (1)
2050-2053: LGTM! German translations added for new analytics categories.The German translations are appropriate and maintain consistency with the English version.
packages/transactional/package.json (1)
11-11: Verify the design system version update impact.The
@uzh-bf/design-systemdependency has been updated from 3.0.0-alpha.42 to 3.0.0-alpha.45.Let's verify the consistency of the design system version across all package.json files:
✅ Verification successful
Design system version update verified successfully
The @uzh-bf/design-system dependency has been consistently updated to version 3.0.0-alpha.45 across all packages. As this is a minor alpha version bump, the update should be safe, but standard testing procedures are recommended to catch any potential visual or functional regressions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify design system version consistency across all package.json files # and check for any breaking changes in the version update # Check all package.json files for design system version echo "Checking design system version across all package.json files..." rg -A 1 '"@uzh-bf/design-system"' '**/package.json' # Look for any migration guides or breaking changes echo "Checking for migration guides or breaking changes..." rg -i "breaking.*change" "CHANGELOG.md"Length of output: 401
Script:
#!/bin/bash # Find all package.json files and check for design system dependency echo "Checking design system versions in package.json files..." fd package.json --type f --exec grep -A 1 "@uzh-bf/design-system" {} \; # Look for any documentation files that might contain breaking changes echo -e "\nChecking for breaking changes in documentation..." fd -e md -e mdx --type f --exec grep -l -i "breaking.*changes\|migration" {} \; # Check actual usage of design system components echo -e "\nChecking design system component usage..." rg -l "@uzh-bf/design-system"Length of output: 25213
apps/func-incoming-responses/package.json (1)
19-19: LGTM! Design system version update is consistent.The
@uzh-bf/design-systemdependency update matches other package updates in the PR.apps/func-response-processor/package.json (1)
21-21: Verify compatibility with other dependencies.The update to
@uzh-bf/[email protected]is an alpha version. Since this is used as a development dependency, ensure it's compatible with other related packages.Run this script to check for potential version conflicts:
✅ Verification successful
Version compatibility verified ✓
All packages in the monorepo are using the same version (3.0.0-alpha.45) of @uzh-bf/design-system, ensuring consistent dependency management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential version conflicts with @uzh-bf/design-system # Look for other packages that might depend on different versions of @uzh-bf/design-system # Search for dependencies on @uzh-bf/design-system across all package.json files fd package.json --exec jq -r 'select(.dependencies."@uzh-bf/design-system" or .devDependencies."@uzh-bf/design-system" or .peerDependencies."@uzh-bf/design-system") | "\(.name): \(.dependencies."@uzh-bf/design-system" // .devDependencies."@uzh-bf/design-system" // .peerDependencies."@uzh-bf/design-system")"'Length of output: 832
apps/docs/package.json (1)
25-25: Verify documentation site compatibility.The update to
@uzh-bf/[email protected]in the documentation project should be tested to ensure no visual regressions in the documentation site.Run this script to check for design system component usage in documentation:
apps/frontend-control/package.json (1)
20-20: Test UI components after design system update.Since this is a frontend application and the PR involves navigation changes, ensure that all UI components using
@uzh-bf/design-systemare thoroughly tested after the update to version3.0.0-alpha.45.Run this script to identify affected components:
apps/frontend-manage/src/components/common/Header.tsx (1)
85-113:⚠️ Potential issueEnsure safe usage of spread operator with potentially undefined arrays
When using the spread operator with
courses?.slice(0, 5).map(...), ifcoursesisundefined, it could cause a runtime error because spreadingundefinedis not allowed. To prevent this, ensure thatcoursesis always an array.Initialize
coursesto an empty array if it's undefined:- const courses = courseData?.userCourses + const courses = courseData?.userCourses ?? []This way,
courses.slice(0, 5).map(...)will not throw an error even ifcoursesis undefined.Likely invalid or redundant comment.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
new-navigation-manage
|
| Run status |
|
| Run duration | 17m 24s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
272
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/frontend-manage/src/components/common/Header.tsx (1)
85-113: Consider extracting course analytics menu configurationThe nested course analytics menu configuration is complex and could benefit from extraction into a separate function for better maintainability.
+ const getCourseAnalyticsMenu = (course: Course) => ({ + key: `course-analytics-${course.id}`, + type: 'submenu' as const, + label: course.name, + data: { cy: `course-analytics-menu-${course.name}` }, + options: [ + { + key: `activity-dashboard-${course.name}`, + type: 'link' as const, + label: t('manage.analytics.activity'), + onClick: () => router.push(`/analytics/${course.id}/activity`), + }, + // ... other options + ], + }) elements: [ - ...(courses?.slice(0, 5).map((course) => ({ - key: `course-analytics-${course.id}`, - type: 'submenu', - // ... rest of the configuration - })) ?? []), + ...(courses?.slice(0, 5).map(getCourseAnalyticsMenu) ?? []),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/frontend-manage/src/components/common/Header.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: SonarCloud
- GitHub Check: build
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/frontend-manage/src/components/common/Header.tsx (4)
30-30: Address TODO comment about query generalizationThe comment indicates that the collection sharing requests query needs to be generalized for the new catalogue.
Would you like me to help implement the generalized query for the new catalogue?
31-36: Verify the impact of cache-first fetch policyThe
cache-firstfetch policy has been added to both queries. While this can improve performance, ensure that it doesn't prevent users from seeing real-time updates to their quizzes and courses.Consider using
cache-and-networkif real-time updates are important for these queries.
207-213: Address TODO comment about favicon replacementThe comment indicates that the favicon needs to be replaced with a high-resolution version with specific requirements.
Would you like me to help create a task for updating the favicon with the specified requirements:
- High resolution
- Transparent background
- Different gray logo
202-204: Verify keyboard navigation functionalityWhile the navigation implementation looks good, please ensure that keyboard navigation works correctly across all menu items and dropdowns.
Test the following scenarios:
- Tab navigation through all menu items
- Enter/Space to activate buttons
- Arrow keys for dropdown navigation
- Escape key to close dropdowns
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/frontend-manage/src/components/common/Header.tsx (3)
85-113: Consider pagination or scrolling for courses in analytics dropdown.The current implementation limits the course list to 5 items (
courses?.slice(0, 5)). This might not be optimal if a user has many courses.Consider implementing one of these solutions:
- Add scrolling to the dropdown menu
- Implement pagination
- Show a "View More" option that expands the list
Would you like me to provide an implementation for any of these solutions?
207-213: Enhance image accessibility and navigation.The clickable logo should be wrapped in a button or anchor tag for better accessibility. Also, consider using Next.js Link component for consistent navigation.
- <Image - src="/img/klicker_icon_transparent.png" - alt="KlickerUZH Logo" - width={35} - height={35} - onClick={() => router.push('/')} - className="hover:cursor-pointer" - /> + <Link href="/" aria-label="Home"> + <Image + src="/img/klicker_icon_transparent.png" + alt="KlickerUZH Logo" + width={35} + height={35} + className="hover:cursor-pointer" + /> + </Link>
202-204: Add ARIA landmark for navigation.The navigation container should have an ARIA role for better accessibility.
<div className="flex h-full w-full flex-row items-center justify-between border-b border-slate-300 bg-slate-100 font-bold text-slate-700 print:!hidden" data-cy="navigation" + role="navigation" + aria-label="Main" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/frontend-manage/public/img/klicker_icon_transparent.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/auth/package.json(1 hunks)apps/backend-docker/package.json(1 hunks)apps/docs/package.json(1 hunks)apps/frontend-control/package.json(1 hunks)apps/frontend-manage/package.json(1 hunks)apps/frontend-manage/src/components/common/Header.tsx(2 hunks)apps/frontend-pwa/package.json(1 hunks)apps/func-incoming-responses/package.json(1 hunks)apps/func-response-processor/package.json(1 hunks)apps/office-addin/package.json(1 hunks)packages/markdown/package.json(1 hunks)packages/shared-components/package.json(1 hunks)packages/transactional/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/docs/package.json
- apps/frontend-manage/package.json
- apps/auth/package.json
- apps/office-addin/package.json
- apps/func-incoming-responses/package.json
- apps/backend-docker/package.json
- packages/transactional/package.json
- packages/shared-components/package.json
- apps/frontend-pwa/package.json
- packages/markdown/package.json
- apps/frontend-control/package.json
- apps/func-response-processor/package.json
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: SonarCloud
- GitHub Check: check
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/frontend-manage/src/components/common/Header.tsx (3)
30-30: Address the TODO comment regarding query generalization.The comment indicates a need to generalize the collection sharing requests query for the new catalogue. This should be addressed before the PR is merged.
Would you like me to help implement the generalized query for the new catalogue?
31-36: LGTM! Appropriate fetch policy for navigation data.The use of
cache-firstfetch policy is well-suited for navigation-related data that doesn't require frequent updates.
144-150: LGTM! Fixed dropdown disable logic.The condition
!quizzes || quizzes.length === 0correctly handles both undefined and empty cases. The conditional styling is also properly implemented.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/frontend-pwa/src/components/docs/DocsLayout.tsx (1)
50-50: Avoid using Tailwind's important modifier (!)The
!rounded-b-noneclass uses Tailwind's important modifier, which can lead to maintainability issues. Consider managing this style through proper component composition or CSS specificity instead.- className={{ root: 'w-full !rounded-b-none bg-slate-200' }} + className={{ root: 'w-full rounded-none bg-slate-200' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/auth/package.json(1 hunks)apps/backend-docker/package.json(1 hunks)apps/docs/package.json(1 hunks)apps/frontend-control/package.json(1 hunks)apps/frontend-manage/package.json(1 hunks)apps/frontend-pwa/package.json(1 hunks)apps/frontend-pwa/src/components/docs/DocsLayout.tsx(1 hunks)apps/func-incoming-responses/package.json(1 hunks)apps/func-response-processor/package.json(1 hunks)apps/office-addin/package.json(1 hunks)packages/markdown/package.json(1 hunks)packages/shared-components/package.json(1 hunks)packages/transactional/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/frontend-pwa/package.json
- apps/docs/package.json
- packages/shared-components/package.json
- apps/auth/package.json
- apps/backend-docker/package.json
- packages/markdown/package.json
- apps/office-addin/package.json
- apps/func-incoming-responses/package.json
- apps/frontend-control/package.json
- packages/transactional/package.json
- apps/func-response-processor/package.json
- apps/frontend-manage/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: SonarCloud
- GitHub Check: cypress-run
🔇 Additional comments (1)
apps/frontend-pwa/src/components/docs/DocsLayout.tsx (1)
92-92: LGTM! Well-structured content stylingThe content wrapper styling is well-implemented using Tailwind's prose classes with appropriate customizations for images and spacing.
Before merging, complete the following tasks:
Summary by CodeRabbit
Dependency Updates
@uzh-bf/design-systemto version3.0.0-alpha.47across multiple packages and applicationsLocalization
UI Improvements