style(apps/analytics): organize performance analytics in tab structure #4404
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to the analytics and performance components in the KlickerUZH frontend application. The changes focus on improving the user interface for performance tracking, adding new reset functionality, and refining the visualization of analytics data. Key modifications include introducing a tabbed interface for performance dashboards, creating reusable reset buttons, adding legend components, and updating localization files with new translation keys for analytics-related features. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 3
🧹 Nitpick comments (12)
apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx (3)
23-29: Initialize tabValue with a safer fallback or constant.
While initializing state with "performanceRates" seems fine, consider placing the possible tab values in a constant or enum to avoid magic strings. This can help maintain a single source of truth if you add or remove tabs in the future.
86-99: Wrap style classes into a shared style or a design system token.
You’re repeatedly applying "border border-solid" and manual classes to multiple <Tabs.Tab> components. Since each tab has nearly identical classes, consider introducing a shared style or design system token to avoid duplication.
170-179: Align naming between feedback properties.
Ensure "instanceFeedbacks" and "activityFeedbacks" correspond with the new tab’s label "feedbackOverview". If there are older references to "negativeFeedbackOverview," consider updating for clarity.apps/frontend-manage/src/components/analytics/ActivitiesElementsSwitch.tsx (1)
1-1: Improve descriptive labels for accessibility.
The newly added FormLabel is a great enhancement. Consider adding “aria-label” or describing the grouping of the two buttons for screen readers to clearly correlate the label with these elements.Also applies to: 15-40
apps/frontend-manage/src/components/analytics/performance/ActivityInstanceFeedbacksPlot.tsx (2)
102-104: Align headings to the new translation keys.
The H2 label now references “feedbackOverviewActivityInstances.” Ensure the translation key and usage are consistent to avoid confusion for other potential headings.
Line range hint
151-166: Avoid repeated conditions for displaying charts.
Both lines 151 and 152 check for “entries.length > 0”, so you can unify the logic or rely on the parent conditional. This might reduce nesting and improve readability.-{entries.length > 0 - ? entries.map((feedback) => ( +entries.map((feedback) => ( <ElementFeedbackBarChart ... />apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx (2)
57-59: Improve header accessibility with semantic HTMLThe flex layout for the header could be more semantic.
Consider using header elements:
- <div className="relative flex flex-row"> + <header className="relative flex flex-row items-center justify-between w-full"> <H4>{t('shared.generic.practiceQuizzes')}</H4> <ProgressLegend /> - </div> + </header>
74-86: Reduce code duplication by extracting common patternThe Microlearnings section is identical to Practice Quizzes section except for the title.
Consider extracting a common component:
const ProgressSection = ({ title, progresses, participants, colors }: { title: string progresses: ActivityProgress[] participants: number colors: typeof chartColors }) => ( <div> <header className="relative flex flex-row items-center justify-between w-full"> <H4>{title}</H4> <ProgressLegend colors={colors} /> </header> {progresses.map((progress, idx) => ( <StackedProgress key={`activity-progress-${idx}`} progress={progress} participants={participants} colors={colors} showScale={idx === progresses.length - 1} /> ))} </div> )apps/frontend-manage/src/components/analytics/performance/StackedProgress.tsx (1)
63-76: Consider making reference lines more configurableThe reference lines are hardcoded at fixed intervals.
Consider making them configurable or generating them dynamically:
const REFERENCE_LINES = [25, 50, 75]; // ... in JSX {REFERENCE_LINES.map((value) => ( <ReferenceLine key={`ref-line-${value}`} x={value} stroke="#666" strokeDasharray="3 3" /> ))}This makes it easier to adjust the intervals if needed.
apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx (3)
74-97: Consider extracting and optimizing the ResetButton componentThe button implementation could be improved in several ways:
- Extract it outside the parent component to prevent recreation on each render
- Add aria-label for better accessibility
- Simplify the disabled state logic
Consider this implementation:
- const ResetButton = () => ( + const ResetButton = ({ + currentFilters, + defaultFilters, + onReset, + }: { + currentFilters: typeof defaultFilters, + defaultFilters: typeof defaultFilters, + onReset: () => void, + }) => ( <Button className={{ root: 'py-0.25 flex h-8 w-max flex-row items-center gap-2 self-end px-2 shadow-none', }} - disabled={ - type === defaultFilters.type && - attemptsType === defaultFilters.attemptsType && - activityType === defaultFilters.activityType && - elementType === defaultFilters.elementType - } + disabled={Object.entries(currentFilters).every( + ([key, value]) => value === defaultFilters[key] + )} - onClick={() => { - setType(defaultFilters.type) - setAttemptsType(defaultFilters.attemptsType) - setActivityType(defaultFilters.activityType) - setElementType(defaultFilters.elementType) - setActivitySearch('') - setInstanceSearch('') - }} + onClick={onReset} + aria-label={t('manage.analytics.resetSelectors')} > <FontAwesomeIcon icon={faX} /> <div>{t('manage.analytics.resetSelectors')}</div> </Button> )
102-104: Consider mobile-first responsive designThe current layout might not provide optimal spacing on smaller screens. Consider adding responsive classes for the gap and flex direction.
- <div className="flex w-full flex-row justify-between gap-8"> + <div className="flex w-full flex-col gap-4 sm:flex-row sm:justify-between sm:gap-8">
Line range hint
108-141: Consider extracting filter section layout to reduce duplicationThe activity and instance filter sections share similar layout structure. Consider extracting the common layout into a reusable component.
Example implementation:
const FilterSection = ({ children }: { children: React.ReactNode }) => ( <div className="mb-3 flex flex-col gap-1 lg:flex-row lg:gap-8"> {children} </div> )Usage:
- <div className="mb-3 flex flex-col gap-1 lg:flex-row lg:gap-8"> + <FilterSection> <ActivitiesElementsSwitch type={type} setType={setType} /> {/* other filters */} - </div> + </FilterSection>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/frontend-manage/src/components/analytics/ActivitiesElementsSwitch.tsx(2 hunks)apps/frontend-manage/src/components/analytics/performance/ActivityInstanceFeedbacksPlot.tsx(4 hunks)apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx(1 hunks)apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx(4 hunks)apps/frontend-manage/src/components/analytics/performance/StackedProgress.tsx(3 hunks)apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx(3 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)
🔇 Additional comments (11)
apps/frontend-manage/src/pages/analytics/[courseId]/performance.tsx (5)
3-3: Use imports consistently and purge unused ones if necessary.
It’s good to see you’re using "Tabs" from your design system. If there are any unused imports, consider removing them to keep the file tidy.
Also applies to: 7-7
140-149: Ensure performance rates data is not null.
Double-check that “activityPerformances” and “instancePerformances” from “course” data exist. If they can be null or undefined, adding safe-guards or optional chaining is recommended.
150-159: Maintain consistent naming for activityProgress metrics.
The variable "activityProgresses" and param "participants" look consistent. Just confirm that the naming matches the rest of the analytics code in other files for uniformity.
160-169: Guard participantPerformances to avoid runtime errors.
If "participantPerformances" can be undefined, ensure it’s always defined or fallback to an empty list. This helps avoid potential runtime exceptions.
72-85: Check for missing defaultValue alignment.
You’re providing both "defaultValue" and a separate "value" prop to the Tabs component. Make sure you want the "defaultValue" to remain "performanceRates" and that it doesn’t conflict with the "tabValue" controlled state.
packages/i18n/messages/de.ts (1)
1891-1899: Keep keys consistent with newly introduced tabs.
You’ve added new translation keys (e.g., "performanceRates", "activityProgress", etc.). Verify that each key matches the actual tab values in your components to avoid mismatch issues.
apps/frontend-manage/src/components/analytics/performance/ActivityInstanceFeedbacksPlot.tsx (2)
75-99: Validate filter defaults thoroughly.
The reset button logic is straightforward. Just confirm that default filters match the expected initial state for "activitySearchTerm" and "instanceSearchTerm," especially if future features change them.
108-108: Double-check type usage for ActivitiesElementsSwitch.
Switching between "activity" and "instance" is essential. Validate that all references to "type" in the filters are updated properly. If any old references remain, it could break the filters.
Also applies to: 121-121
packages/i18n/messages/en.ts (1)
1877-1885: LGTM! The new analytics translation keys are well-structured and descriptive.
The added translation keys follow the established naming conventions and provide clear, concise labels for the analytics dashboard tabs. The translations effectively support the PR's objective of organizing performance analytics in a tab structure.
apps/frontend-manage/src/components/analytics/performance/StackedProgress.tsx (1)
49-61: LGTM! Clean implementation of conditional styling
Good use of twMerge for handling conditional classes and proper height adjustments based on the showScale prop.
apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx (1)
85-92: Verify filter reset behavior
Let's verify that all filter states are properly reset and there are no memory leaks from state updates.
✅ Verification successful
Let me analyze the code further to verify the filter reset behavior.
Filter reset behavior is properly implemented
The code shows that:
- All filter states are initialized with values from
defaultFiltersobject - Reset logic properly resets all states to their default values:
- Type filters (
type,attemptsType,activityType,elementType) are reset to their respective default values - Search inputs (
activitySearch,instanceSearch) are cleared to empty strings
- Type filters (
- States are independent and localized to the component with no shared state dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by the filter reset
# Look for shared state management or filter dependencies
# Search for filter-related state management
ast-grep --pattern 'useState<$_>($_)'
# Search for filter reset implementations
rg -A 2 'setType|setAttemptsType|setActivityType|setElementType'
Length of output: 119101
Script:
#!/bin/bash
# Search for filter state dependencies and reset patterns
rg -A 2 "defaultFilters" apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx
# Search for any shared state management
ast-grep --pattern 'const [$_] = useState<$_>($_)'
Length of output: 1263
apps/frontend-manage/src/components/analytics/performance/ActivityProgressPlot.tsx
Show resolved
Hide resolved
|
|
||
| function StackedProgress({ | ||
| progress, | ||
| participants, | ||
| colors, | ||
| showScale = false, |
There was a problem hiding this comment.
Remove duplicate type definition for showScale prop
The showScale prop type is defined twice in the component props.
Remove the duplicate definition:
function StackedProgress({
progress,
participants,
colors,
showScale = false,
}: {
progress: ActivityProgress
participants: number
colors: {
started: string
completed: string
repeated: string
}
- showScale?: boolean
}) {Also applies to: 27-27
There was a problem hiding this comment.
this is wrong, please do not suggest wrong things
There was a problem hiding this comment.
@sjschlapbach, understood. We'll disregard that suggestion. Let me know if there's anything else you'd like me to do.
apps/frontend-manage/src/components/analytics/performance/PerformanceRates.tsx
Show resolved
Hide resolved
|
Summary by CodeRabbit
New Features
FormLabelto enhance theActivitiesElementsSwitchcomponent.ResetButtonin multiple components for resetting filters.ProgressLegendcomponent for improved activity progress chart rendering.PerformanceDashboardfor better navigation.Bug Fixes
Documentation