-
Notifications
You must be signed in to change notification settings - Fork 3
feat: added secondary alphabetical sort on timezone selector #1089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe timezone selector component was refactored to replace the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/Timezone Selector/index.tsx (1)
6-11: Remove unused props from the interface or implement their functionality.The interface declares
onChange,label, andisEditableprops, but line 13 only destructuresvalue. These props are never used:
onChange: The component uses an internalhandleChangethat makes direct API calls instead of calling this proplabel: Never referenced in the componentisEditable: Never used;isDisabledis hardcoded tofalseon line 78This creates a misleading API where consumers might pass these props expecting them to control component behavior.
🔎 Proposed fix: Remove unused props from interface
interface TimezoneSelectorProps { value: string - onChange: (value: string) => void - label?: string - isEditable?: boolean }Or implement the prop functionality if they're intended to be used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Timezone Selector/index.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (4)
components/Timezone Selector/index.tsx (4)
76-76: No changes needed. TheparseTimezonefunction fromuseTimezoneSelectis typed to returnITimezoneOption, and thevalueprop accepts either a string orITimezoneOption. The usage at line 76 is correct and compatible with react-select.
18-18: No changes needed –displayValue: 'UTC'is a valid and documented option.The
displayValueprop accepts'GMT'or'UTC'as valid values, and the current implementation usingdisplayValue: 'UTC'is correct and achieves the intended formatting of timezone labels.
20-26: Sorting logic is correct.The secondary alphabetical sort uses
a.value.localeCompare(b.value), which correctly sorts by the timezone identifier (e.g., "America/Juneau", "Europe/Berlin") rather than the formatted display label. The implementation properly:
- Creates a copy of the options array to avoid mutation
- Uses nullish coalescing for defensive handling of undefined offsets
- Applies
localeComparefor proper internationalization support in alphabetical ordering
1-3: The implementation approach is intentional and supported by the library design.By default, react-timezone-select uses react-select as its underlying select component. The library explicitly provides a useTimezoneSelect hook to decouple the component from the react-select dependency, enabling developers to use the hook's timezone utilities with external Select components. The useTimezoneSelect hook is documented with examples showing it can work with native select elements or custom implementations, including react-select. This is not an unconventional approach—it's a supported design pattern provided by the library.
Description
The secondary sort is seemingly random after the UTC hour on the timezone selector (/account page) so this fixes that.
Test plan
Go to /account and see that we're sorting alphabetically on timezones with the same UTC hour offset.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.