-
Notifications
You must be signed in to change notification settings - Fork 339
add range select unlock #2420
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
base: main
Are you sure you want to change the base?
add range select unlock #2420
Conversation
📝 WalkthroughWalkthroughThis change adds Daily time period support to UpcomingUnlockVolumeChart, expanding the TIME_PERIODS configuration from two to three options. The grouping logic is updated to handle Daily, Weekly, and Monthly buckets across both view modes, with a new groupBy prop passed to BarChart. The UI control is refactored from a single selector to separate TimePeriod and ViewMode TagGroup components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
src/components/Charts/UpcomingUnlockVolumeChart.tsx (2)
87-92: Consider extracting duplicated grouping key logic.This ternary chain is duplicated at lines 114-119. Extract to a helper for maintainability:
♻️ Proposed refactor
Add a helper function before the component:
function getGroupingKey(date: dayjs.Dayjs, timePeriod: TimePeriod): number { if (timePeriod === 'Daily') return date.startOf('day').unix() if (timePeriod === 'Weekly') return date.startOf('week').unix() return date.startOf('month').unix() }Then replace both occurrences with:
-const key = - timePeriod === 'Daily' - ? date.startOf('day').unix() - : timePeriod === 'Weekly' - ? date.startOf('week').unix() - : date.startOf('month').unix() +const key = getGroupingKey(date, timePeriod)
168-179: Prefer spread operator over double type casting.The
as unknown as string[]pattern is awkward. Use the spread operator for cleaner conversion:♻️ Proposed refactor
<TagGroup selectedValue={timePeriod} setValue={(value: TimePeriod) => setTimePeriod(value)} - values={TIME_PERIODS as unknown as string[]} + values={[...TIME_PERIODS]} /> <TagGroup selectedValue={viewMode} setValue={(value: ViewMode) => setViewMode(value)} - values={VIEW_MODES as unknown as string[]} + values={[...VIEW_MODES]} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Charts/UpcomingUnlockVolumeChart.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Charts/UpcomingUnlockVolumeChart.tsx (1)
src/components/TagGroup.tsx (1)
TagGroup(16-63)
🔇 Additional comments (3)
src/components/Charts/UpcomingUnlockVolumeChart.tsx (3)
30-31: LGTM!The expansion of TIME_PERIODS to include 'Daily' is correctly typed with
as const, and the TimePeriod type is properly derived.
166-166: LGTM!The lowercase conversion correctly maps TimePeriod values to the expected groupBy union type.
38-38: Verify:isFullViewstate has no toggle control.The
isFullViewstate is declared and used in the data filtering logic (line 56) but there's no UI to change it. Is this intentional for a future feature, or should a toggle be added alongside the new controls?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.