[Feat]: Update DynamicSelect component and localization Files #3334
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces localization enhancements across multiple components related to timesheet filtering. Key changes include the implementation of translation functions for various filter-related labels in the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 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: 7
🧹 Outside diff range and nitpick comments (8)
apps/web/lib/components/custom-select/select-items.tsx (1)
123-123: Avoid hardcoding z-index valuesUsing hardcoded z-index values like 'z-[10000]' can lead to stacking context issues. Consider using CSS custom properties or a z-index management system.
-<SelectContent className='z-[10000]'> +<SelectContent className='z-select-dropdown'>apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (2)
111-112: Improve clear filter implementationThe current clear implementation using
shouldRemoveItemsboolean and useEffect is indirect. Consider:
- Implementing a direct clear method
- Adding loading state during clear operation
- Handling potential errors
<Button - onClick={() => setShouldRemoveItems(true)} + onClick={async () => { + try { + setIsClearing(true); + await clearAllFilters(); + } catch (error) { + console.error('Failed to clear filters:', error); + } finally { + setIsClearing(false); + } + }} variant={'outline'} - className='flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300' + className='flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300' + disabled={isClearing} > - <span className="text-sm">{t('common.CLEAR_FILTER')}</span> + <span className="text-sm"> + {isClearing ? t('common.CLEARING') : t('common.CLEAR_FILTER')} + </span> </Button>
Line range hint
52-58: Improve MultiSelect implementationsCurrent implementation has several issues:
- Console.log statements in production code
- Duplicate MultiSelect configurations
- Missing error handling
Consider extracting a common MultiSelect configuration and implementing proper handlers:
// Create a shared configuration interface FilterMultiSelectProps<T> { items: T[]; itemToString: (item: T) => string; itemId: (item: T) => string; onValueChange: (items: T[]) => void; removeItems?: boolean; } function FilterMultiSelect<T>({ items, itemToString, itemId, onValueChange, removeItems }: FilterMultiSelectProps<T>) { return ( <MultiSelect removeItems={removeItems} items={items} itemToString={itemToString} itemId={itemId} onValueChange={(selectedItems) => { try { onValueChange(selectedItems); } catch (error) { console.error('Error handling selection change:', error); } }} multiSelect={true} triggerClassName="dark:border-gray-700" /> ); }Then use it like:
-<MultiSelect - removeItems={shouldRemoveItems} - items={activeTeam?.members ?? []} - itemToString={(members) => (members ? members.employee.fullName : '')} - itemId={(item) => item.id} - onValueChange={(selectedItems) => console.log(selectedItems)} - multiSelect={true} - triggerClassName="dark:border-gray-700" -/> +<FilterMultiSelect + items={activeTeam?.members ?? []} + itemToString={(members) => (members ? members.employee.fullName : '')} + itemId={(item) => item.id} + onValueChange={handleMemberSelection} + removeItems={shouldRemoveItems} +/>Also applies to: 65-72, 81-88, 96-103
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (1)
Line range hint
243-244: Add accessibility attributes to calendar iconThe calendar icon button should have proper accessibility attributes for screen readers.
- <PiCalendarDotsThin className="h-5 w-5 dark:text-gray-500" /> + <PiCalendarDotsThin className="h-5 w-5 dark:text-gray-500" aria-hidden="true" role="presentation" />apps/web/locales/nl.json (1)
265-268: LGTM! Consider adding descriptions for filter actions.The new filter-related translations are well-structured and appropriately placed in the
commonsection. The translations are concise and clear.Consider adding descriptions or tooltips for these filter actions to enhance user experience. For example:
{ "common": { "FILTER_CUSTOM_RANGE": "Aangepast bereik", - "CLEAR_FILTER": "Filter wissen", + "CLEAR_FILTER": "Filter wissen", + "CLEAR_FILTER_DESCRIPTION": "Alle filterinstellingen verwijderen", - "CLEAR": "Wissen", + "CLEAR": "Wissen", + "CLEAR_DESCRIPTION": "Huidige selectie wissen", - "APPLY_FILTER": "Filter toepassen" + "APPLY_FILTER": "Filter toepassen", + "APPLY_FILTER_DESCRIPTION": "De geselecteerde filterinstellingen toepassen" } }apps/web/locales/es.json (1)
265-268: LGTM! The translations are clear and consistent.The new filter-related translations are properly added under the common section and follow the existing naming conventions. The Spanish translations are accurate and appropriate for their intended use in filtering functionality.
Consider adding a period at the end of each translation to match the punctuation style used in other translations throughout the file, like this:
- "FILTER_CUSTOM_RANGE": "Rango personalizado", - "CLEAR_FILTER": "Borrar filtro", - "CLEAR": "Borrar", - "APPLY_FILTER": "Aplicar filtro" + "FILTER_CUSTOM_RANGE": "Rango personalizado.", + "CLEAR_FILTER": "Borrar filtro.", + "CLEAR": "Borrar.", + "APPLY_FILTER": "Aplicar filtro."apps/web/locales/de.json (1)
265-268: LGTM! The translations are accurate and follow German language conventions.The new filter-related translations are grammatically correct and consistent with common German UI terminology. However, for better consistency across the application, consider using the same word stem for related actions:
"FILTER_CUSTOM_RANGE": "Benutzerdefinierter Bereich", -"CLEAR_FILTER": "Filter löschen", +"CLEAR_FILTER": "Filter zurücksetzen", -"CLEAR": "Löschen", +"CLEAR": "Zurücksetzen", "APPLY_FILTER": "Filter anwenden"This suggestion uses "zurücksetzen" (reset) instead of "löschen" (delete) to better align with the temporary nature of clearing filters, as the data isn't permanently deleted.
apps/web/locales/fr.json (1)
265-268: LGTM! The translations are accurate and well-structured.The French translations for filter-related actions are grammatically correct and maintain consistency with the existing translations.
Consider adding a period at the end of each translation to match the style of some other translations in the file, though this is purely stylistic:
- "FILTER_CUSTOM_RANGE": "Plage personnalisée", - "CLEAR_FILTER": "Effacer le filtre", - "CLEAR": "Effacer", - "APPLY_FILTER": "Appliquer le filtre" + "FILTER_CUSTOM_RANGE": "Plage personnalisée.", + "CLEAR_FILTER": "Effacer le filtre.", + "CLEAR": "Effacer.", + "APPLY_FILTER": "Appliquer le filtre."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx(5 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(1 hunks)apps/web/lib/components/custom-select/select-items.tsx(2 hunks)apps/web/lib/features/manual-time/add-manual-time-modal.tsx(0 hunks)apps/web/locales/ar.json(1 hunks)apps/web/locales/bg.json(1 hunks)apps/web/locales/de.json(1 hunks)apps/web/locales/en.json(1 hunks)apps/web/locales/es.json(1 hunks)apps/web/locales/fr.json(1 hunks)apps/web/locales/he.json(1 hunks)apps/web/locales/it.json(1 hunks)apps/web/locales/nl.json(1 hunks)apps/web/locales/pl.json(1 hunks)apps/web/locales/pt.json(1 hunks)apps/web/locales/ru.json(1 hunks)apps/web/locales/zh.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/lib/features/manual-time/add-manual-time-modal.tsx
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx (1)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx:30-35
Timestamp: 2024-11-12T14:06:02.202Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
🔇 Additional comments (13)
apps/web/lib/components/custom-select/select-items.tsx (1)
6-14: LGTM: Imports are well-organized
The Select-related component imports are properly structured and grouped together.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (2)
178-178: Consider dark mode text contrast on hover
The hover state styling looks good, but consider testing the white text color (hover:text-white) against the hover:dark:bg-primary-light background in dark mode to ensure sufficient contrast ratio for accessibility.
#!/bin/bash
# Check for any other instances of similar hover styling patterns
rg "hover:dark:bg-primary-light.*hover:text-white|hover:text-white.*hover:dark:bg-primary-light" --type tsxLine range hint 313-318: Standardize date handling library usage
The component uses both date-fns and moment.js. Consider standardizing on date-fns since:
- It's already used elsewhere in the codebase
- Moment.js is considered legacy and is significantly larger
- Using multiple date libraries increases bundle size
Example refactor using date-fns:
- const createDateKey = (date: string | Date) =>
- moment(date.toString().split('T')[0]).toISOString().split('T')[0];
+ const createDateKey = (date: string | Date) =>
+ format(new Date(date.toString().split('T')[0]), 'yyyy-MM-dd');apps/web/locales/zh.json (1)
265-268: LGTM! Translations are semantically consistent.
The new filter-related translations maintain semantic consistency by using "筛选" consistently for "filter" related terms, and the translations are clear and concise.
apps/web/locales/he.json (1)
265-268: LGTM! The new filter-related translations are well-structured.
The added Hebrew translations for filter actions are clear, consistent with RTL language requirements, and follow the established localization patterns.
apps/web/locales/ar.json (1)
247-250: LGTM! The Arabic translations for filter-related strings are accurate and consistent.
The new translations are grammatically correct, maintain terminology consistency, and properly support the filtering functionality.
apps/web/locales/en.json (1)
265-268: LGTM! The new filter-related translations are consistent.
The new translation keys follow the established naming patterns and provide clear, concise translations for the timesheet filtering functionality.
Let's verify consistency with other filter-related translations:
✅ Verification successful
LGTM! The new filter-related translations are consistent across all localization files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in filter-related translation keys
# Expected: All filter-related keys should follow similar naming patterns
# Search for all filter-related keys in the localization files
rg '"FILTER_' apps/web/locales/
Length of output: 7245
apps/web/locales/bg.json (1)
265-268: LGTM! Translations are consistent and well-structured.
The new filter-related translations maintain consistent terminology with the existing translations and follow the established naming conventions.
Let's verify the consistency of these translations across other localization files:
✅ Verification successful
All localization files include the new filter-related keys consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new filter-related keys are consistently present across all localization files
# Expected: All localization files should have these keys to ensure complete translations
# Find all locale files
fd -e json -d 1 . apps/web/locales/ --exec sh -c '
echo "=== Checking $1 ==="
# Check for the presence of new filter keys
jq -r ".common | select(.CLEAR_FILTER != null and .CLEAR != null and .APPLY_FILTER != null) | \"Found all keys\"" "$1"
echo ""
' sh
Length of output: 1017
apps/web/locales/pl.json (1)
265-268: LGTM! The new filter-related translations are well-structured.
The added translations maintain consistency with Polish language conventions and follow the established key naming pattern.
apps/web/locales/it.json (1)
265-268: LGTM! The filter-related translations are well-structured.
The new Italian translations for filter functionality are properly formatted, follow the existing naming conventions, and provide accurate translations for the filter-related actions.
Let's verify the consistency of these translations across other locale files:
✅ Verification successful
Verified: Filter-related translations are consistently present across all locale files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new filter-related keys exist in all locale files
# and follow consistent casing conventions.
# Test: Search for the new filter keys across all locale files
echo "Checking filter-related keys across locale files..."
rg -U '"CLEAR_FILTER"|"CLEAR"|"APPLY_FILTER"' apps/web/locales/
Length of output: 2318
apps/web/locales/ru.json (2)
265-268: LGTM! The Russian translations for the filter-related keys look accurate.
The new translations maintain consistency with the existing translation style and appear to be properly translated to Russian.
265-268: Verify translations exist in other language files.
Let's ensure these new filter-related keys are consistently present across all language files.
✅ Verification successful
Translations Consistently Present
All filter-related translation keys are present in all language files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new filter-related keys exist in all language files
# Expected: All language files should contain these keys
# Find all locale files
echo "Checking localization files for filter-related keys..."
fd -e json . apps/web/locales -x grep -l "FILTER_CUSTOM_RANGE\|CLEAR_FILTER\|CLEAR\|APPLY_FILTER" {} \;
# Show any missing keys in other locale files
echo -e "\nChecking for missing keys..."
for key in "FILTER_CUSTOM_RANGE" "CLEAR_FILTER" "CLEAR" "APPLY_FILTER"; do
echo "Checking key: $key"
fd -e json . apps/web/locales -x grep -L "\"$key\":" {} \;
done
Length of output: 1449
apps/web/locales/pt.json (1)
265-268: LGTM! Verify consistency across language files.
The Portuguese translations for filter-related strings are accurate and maintain proper grammar. The translations align well with common UI patterns in Portuguese applications.
Let's verify that these new filter-related keys are consistently present across all language files:
✅ Verification successful
All language files contain the new filter-related keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all language files have the new filter-related keys
# Expected: All language files should contain these keys
# Find all language files
echo "Checking language files for filter-related keys..."
for key in "FILTER_CUSTOM_RANGE" "CLEAR_FILTER" "CLEAR" "APPLY_FILTER"; do
echo "=== Checking key: $key ==="
rg -l "\"$key\":" apps/web/locales/
done
Length of output: 2194
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/TimeSheetFilterPopover.tsx
Show resolved
Hide resolved
Innocent-Akim
added
feature
Estimate Task
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/web/lib/components/custom-select/select-items.tsx (3)
109-119: Consider enhancing type definition with additional TypeScript featuresThe type definition is good but could be improved for better type safety and flexibility:
type DynamicSelectProps<T> = { items: T[] label: string placeholder: string getItemLabel: (item: T) => string getItemValue: (item: T) => string - onChange?: (value: string) => void + onChange?: (value: string, item: T) => void + className?: string + 'aria-label'?: string disabled?: boolean error?: string defaultValue?: string }
145-145: Avoid using arbitrary z-index valuesThe z-index value of 10000 seems arbitrary and could cause stacking context issues. Consider using a z-index scale from your design system.
-<SelectContent className='z-[10000]'> +<SelectContent className='z-50'>
148-152: Consider virtualizing large lists for better performanceFor large lists, consider using a virtualization library like
react-windoworreact-virtualizedto improve performance.Example implementation with
react-window:import { FixedSizeList } from 'react-window'; const Row = ({ data, index, style }) => { const item = data.items[index]; return ( <SelectItem style={style} key={data.getItemValue(item)} value={data.getItemValue(item)} > {data.getItemLabel(item)} </SelectItem> ); }; // In the render method: {items.length > 100 ? ( <FixedSizeList height={300} itemCount={items.length} itemSize={35} itemData={{ items, getItemValue, getItemLabel }} > {Row} </FixedSizeList> ) : ( items.map((item) => ( <SelectItem key={getItemValue(item)} value={getItemValue(item)}> {getItemLabel(item)} </SelectItem> )) )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/lib/components/custom-select/select-items.tsx(2 hunks)
🔇 Additional comments (2)
apps/web/lib/components/custom-select/select-items.tsx (2)
6-14: LGTM!
The imports are well-organized and follow the project's conventions.
121-160: Overall implementation looks good!
The component is well-structured, follows React best practices, and properly handles accessibility concerns. The memoization will help with performance, and the error handling is comprehensive.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/web/lib/components/custom-select/select-items.tsx (3)
145-145: Consider using a lower z-index valueThe z-index value of 10000 seems arbitrarily high. Consider using a lower value or defining z-index values in a theme configuration to maintain consistency.
-<SelectContent className='z-[10000]'> +<SelectContent className='z-50'>
121-165: Add test coverage for the DynamicSelect componentThe component handles important UI interactions and should have test coverage to ensure reliability. Consider adding tests for:
- Rendering with different prop combinations
- Error state display
- Disabled state behavior
- Selection functionality
- Accessibility features
Would you like me to help create a test file with the necessary test cases?
Incomplete migration of SelectItems to DynamicSelect
apps/web/lib/features/manual-time/add-manual-time-modal.tsxstill usesSelectItems.🔗 Analysis chain
Line range hint
17-165: Clarify component migration strategyThe file contains both
SelectItemsandDynamicSelectcomponents. Consider:
- Are both components needed?
- Is there a plan to migrate all
SelectItemsusage toDynamicSelect?- Should we document when to use each component?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check current usage of both components echo "Checking SelectItems usage..." rg "SelectItems" --type tsx -l echo "Checking DynamicSelect usage..." rg "DynamicSelect" --type tsx -lLength of output: 261
Script:
# New verification script to check usage of SelectItems and DynamicSelect in .tsx files echo "Checking SelectItems usage..." rg "SelectItems" --glob "*.tsx" -l echo "Checking DynamicSelect usage..." rg "DynamicSelect" --glob "*.tsx" -lLength of output: 383
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/lib/components/custom-select/select-items.tsx(2 hunks)
🔇 Additional comments (1)
apps/web/lib/components/custom-select/select-items.tsx (1)
6-14: Verify import path alias consistency
The @/ import path alias is used here. Ensure this alias is consistently used across the codebase for UI component imports.
Innocent-Akim
removed
the
Don't Merge
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
Release Notes
New Features
DynamicSelectcomponent for improved select input customization.Improvements
AddManualTimeModalfor better context-sensitive user interactions.Bug Fixes