feat(meetings): add month sorting | fix #3239 #3282
Conversation
|
Task linked: CU-86c1br7q3 [FEAT] Add month sorting |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces new icon components ( Changes
Sequence DiagramsequenceDiagram
participant User
participant WeekSelector
participant MonthsContainer
User->>WeekSelector: Click sort icon
WeekSelector->>WeekSelector: Toggle sortDown state
WeekSelector->>MonthsContainer: Pass reverse={sortDown}
MonthsContainer->>MonthsContainer: Conditionally reverse months
MonthsContainer-->>User: Display months in new order
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
🧹 Nitpick comments (11)
src/features/meetings/week_selector/months_container/index.types.ts (1)
5-5: Clarify the usage of the "reverse" property for improved maintainability.Consider adding a short JSDoc or inline comment to explain how this property affects the ordering of months.
src/components/icons/IconSortUp.tsx (2)
3-9: Consider reusing a shared icon prop type.You might centralize and reuse the prop type definition (IconProps) across multiple icon components to maintain consistency and reduce duplication.
11-41: Inline usage inside .Although wrapping an in works, removing the nested tag and passing its paths directly to is a recommended MUI practice and can simplify the DOM structure.
-<SvgIcon - className={`organized-icon-sort-up ${className}`} - sx={{ width: `${width}px`, height: `${height}px`, ...sx }} -> - <svg - width="24" - height="24" - viewBox="0 0 24 24" - fill="none" - xmlns="http://www.w3.org/2000/svg" - > - <path d="..." fill={color} /> - <path d="..." fill={color} /> - </svg> -</SvgIcon> +<SvgIcon + className={`organized-icon-sort-up ${className}`} + sx={{ width: `${width}px`, height: `${height}px`, ...sx }} + viewBox="0 0 24 24" +> + <path d="..." fill={color} /> + <path d="..." fill={color} /> +</SvgIcon>src/components/icons/IconSortDown.tsx (2)
3-9: Leverage a shared icon prop type for consistency.Similar to IconSortUp, consider centralizing the prop type definition (IconProps) to improve consistency and scalability across your icon components.
11-41: Avoid nesting within .Following MUI recommendations, inline the paths directly in the to simplify the DOM and keep your SVGs consistent across components.
-<SvgIcon - className={`organized-icon-sort-down ${className}`} - sx={{ width: `${width}px`, height: `${height}px`, ...sx }} -> - <svg - width="24" - height="24" - viewBox="0 0 24 24" - fill="none" - xmlns="http://www.w3.org/2000/svg" - > - <path d="..." fill={color} /> - <path d="..." fill={color} /> - </svg> -</SvgIcon> +<SvgIcon + className={`organized-icon-sort-down ${className}`} + sx={{ width: `${width}px`, height: `${height}px`, ...sx }} + viewBox="0 0 24 24" +> + <path d="..." fill={color} /> + <path d="..." fill={color} /> +</SvgIcon>src/components/tabs/index.types.ts (4)
68-70: Enforce consistent naming conventions for className.Proactively align naming across components (e.g., "className", "containerClassName", etc.) to keep your props uniform.
73-76: Document permissible "variant" values for clarity.Though referencing MUI's
TabsOwnProps['variant'], you could list acceptable variants like"scrollable"or"standard"to help developers avoid guesswork.
79-82: Consider setting a default minimum height.A default
minHeightmight help maintain uniform visuals across tabs when the property is not specified by the consumer.
85-87: Use "sx" consistently with other MUI-based components.Ensure that using the
sxprop here aligns with your styling approach for tabs. Optionally, override top-level styling with a theme-based approach for a unified design system.src/features/meetings/week_selector/useWeekSelector.tsx (1)
22-22: Consider naming consistency for clarity.
While "sortDown" is functional, consider naming it “isDescending” or something more descriptive to align with typical naming conventions.src/features/meetings/week_selector/index.tsx (1)
30-31: Keep descriptive naming for better readability.
“sortDown” indicates descending order, which might be better named “isDescending” or “descendingOrderEnabled.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
converter/svg/sources/name=sort-down.svgis excluded by!**/*.svg,!**/*.svgconverter/svg/sources/name=sort-up.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (8)
src/components/icons/IconSortDown.tsx(1 hunks)src/components/icons/IconSortUp.tsx(1 hunks)src/components/icons/index.ts(1 hunks)src/components/tabs/index.types.ts(1 hunks)src/features/meetings/week_selector/index.tsx(3 hunks)src/features/meetings/week_selector/months_container/index.tsx(2 hunks)src/features/meetings/week_selector/months_container/index.types.ts(1 hunks)src/features/meetings/week_selector/useWeekSelector.tsx(3 hunks)
🔇 Additional comments (9)
src/components/tabs/index.types.ts (1)
60-65: Validate the "activeTab" index before invoking the callback.
Ensure callers correctly handle the case when an unexpected or out-of-range tab index gets passed to the onChange callback.
src/features/meetings/week_selector/useWeekSelector.tsx (3)
29-29: Great integration of the new “reverse” prop.
This enables a clean, easily maintainable approach to controlling month order.
75-76: Well-exposed hook state and toggle function.
Exporting both the state and method ensures flexible usage in the parent component.
45-47: Ensure consistent UI updates when toggling.
Double-check that all dependencies on “sortDown” properly re-render after toggling.
✅ Verification successful
Let me search for any dependencies on the sortDown state to verify its impact on UI updates.
Let me check the MonthsContainer component since it's the one that receives the sortDown state as a prop.
Let me try a different approach to find the MonthsContainer implementation.
Based on the gathered information, I can now provide a final response:
The sortDown toggle implementation is properly handled and safe.
The sortDown state is only used in one place to reverse the order of months in the MonthsContainer component, which performs a simple array reversal using the spread operator. The UI updates are properly managed through React's state management and prop system:
- The state is toggled in
useWeekSelectorhook - Passed as a prop to
MonthsContainer MonthsContainercreates a new array on each render when the prop changes- Each
MonthItemis properly keyed for React reconciliation
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usages of sortDown in the component and its dependencies
rg "sortDown" -A 3 -B 3
Length of output: 2714
Script:
#!/bin/bash
# Look for the component implementation to understand the context
ast-grep --pattern 'const useWeekSelector = () => {
$$$
}'
Length of output: 5822
Script:
#!/bin/bash
# Find the component that uses this hook
rg "useWeekSelector" -A 3 -B 3
Length of output: 4112
Script:
#!/bin/bash
# Look for MonthsContainer implementation
ast-grep --pattern 'const MonthsContainer = ({ months, reverse }: MonthsContainerProps) => {
$$$
}'
Length of output: 103
Script:
#!/bin/bash
# Search for any other components that might use sortDown or reverse prop
rg "reverse={" -A 3 -B 3
Length of output: 108
Script:
#!/bin/bash
# Look for MonthsContainer implementation with a simpler pattern
ast-grep --pattern 'const MonthsContainer = $_'
Length of output: 1889
Script:
#!/bin/bash
# Search for the file content directly
rg -l "MonthsContainer" | xargs cat
Length of output: 3020
Script:
#!/bin/bash
# Search for reverse prop usage with proper escaping
rg "reverse=\{" -A 3 -B 3
Length of output: 600
src/features/meetings/week_selector/index.tsx (2)
2-7: Good addition of sorting icons.
These icons make the UI more intuitive. Just ensure that size and color remain consistent across different themes or display modes.
68-97: Ensure minimal friction in user interactions.
While clicking the sort icon stops event propagation, ensure that this doesn’t prevent other crucial event handlers (e.g., tooltip, analytics) from firing if needed.
✅ Verification successful
Let me search for any event handlers or related functionality around this component.
Let me try a different search approach without specifying the file type.
Let me check if there are any tooltip or analytics-related components being used in this file.
Based on the findings, I can now generate the final response:
Event propagation implementation is appropriate
The current implementation of stopPropagation() is correct and safe. The component doesn't use any tooltips or analytics tracking that could be affected by stopping event propagation. The sort icon's click handler is properly isolated while maintaining the component's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the component file and its usage
rg -l "WeekSelector|week_selector" --type tsx
# Look for any tooltip or analytics related code in the week selector
rg -A 5 "tooltip|analytics|track" src/features/meetings/week_selector/
Length of output: 148
Script:
#!/bin/bash
# Search for the component file and its usage
rg -l "WeekSelector|week_selector"
# Look for event handlers, tooltips, or analytics in the week selector component
rg -A 5 "onClick|onHover|tooltip|analytics|track" src/features/meetings/week_selector/index.tsx
# Search for the handleToggleSort implementation
ast-grep --pattern 'const handleToggleSort = $_'
Length of output: 1452
Script:
#!/bin/bash
# Check for imports and component usage in the week selector
rg -A 3 "import.*from" src/features/meetings/week_selector/index.tsx
# Look for any tooltip components in the codebase
rg -l "Tooltip|tooltip" src/components/
# Check if there are any analytics utilities being imported
rg -l "analytics|track" src/utils/
Length of output: 701
src/components/icons/index.ts (1)
296-297: Icons successfully added.
They’re consistent with naming conventions and complement the existing library. No action needed.
src/features/meetings/week_selector/months_container/index.tsx (2)
5-7: Elegant condition for reversing the months.
Destructuring and reversing the array inline keeps the code concise.
19-19: Map key usage is correct.
Using the month’s “value” as the key is a reasonable approach to avoid potential duplicates.
|
I'm sorry @FussuChalice but I mixed up the icons in Figma. Could you re-import the updated ones? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/icons/IconSortUp.tsx (2)
3-9: Consider improving type safety for className propThe type definition is well-structured, but could be more specific about className expectations.
type IconProps = { color?: string; width?: number; height?: number; sx?: SxProps<Theme>; - className?: string; + className?: string | undefined; };
19-22: Handle undefined className gracefullyThe className concatenation could throw if className is undefined.
- className={`organized-icon-sort-up ${className}`} + className={`organized-icon-sort-up${className ? ` ${className}` : ''}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
converter/svg/sources/name=sort-down.svgis excluded by!**/*.svg,!**/*.svgconverter/svg/sources/name=sort-up.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (2)
src/components/icons/IconSortDown.tsx(1 hunks)src/components/icons/IconSortUp.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/icons/IconSortDown.tsx
🔇 Additional comments (2)
src/components/icons/IconSortUp.tsx (2)
43-43: LGTM!
The export statement follows the project's conventions.
1-43: Verify integration with sorting feature
Let's verify the integration of this icon with the sorting functionality mentioned in PR #3239.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the integration.
Icon is properly integrated with week selector component
The IconSortUp component is correctly integrated:
- Imported and used in the week selector component (
src/features/meetings/week_selector/index.tsx) - Properly exported alongside IconSortDown in the icons index file
- Used with the correct color prop matching the design system (
color="var(--black)")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of IconSortUp in the week selector component
# Check usage in week selector component
rg -A 5 "IconSortUp" "src/features/meetings"
# Verify if both sort icons are exported together
rg "IconSort(Up|Down)" "src/components/icons/index.ts"
Length of output: 1187
There was a problem hiding this comment.
@FussuChalice: could you please add the functionality to store the sorting preference in localStorage and initially load that value when opening this form? Thank you.
There was a problem hiding this comment.
Okay. No problem.
|
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist: