Custom upcoming length #4206
Custom upcoming length #4206
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
WalkthroughThis pull request introduces a new Possibly related PRs
Suggested labels
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: 3
🧹 Nitpick comments (4)
packages/desktop-client/src/components/schedules/UpcomingLength.tsx (3)
54-66: Consider adding loading state and animation for save action.To address the "abrupt" save concern mentioned in the PR description:
const [tempUpcomingLength, setTempUpcomingLength] = useState(upcomingLength); const [useCustomLength, setUseCustomLength] = useState( nonCustomUpcomingLengthValues(tempUpcomingLength), ); const [saveActive, setSaveActive] = useState(false); +const [isSaving, setIsSaving] = useState(false); useEffect(() => { if (tempUpcomingLength !== upcomingLength) { setSaveActive(true); } else { setSaveActive(false); } }, [tempUpcomingLength, upcomingLength]);Then update the save handler:
const saveUpcomingLength = () => { + setIsSaving(true); setUpcomingLength(tempUpcomingLength); + setTimeout(() => { + setIsSaving(false); + close(); + }, 500); };
107-112: Add ARIA labels for better accessibility.The custom length section should be properly labeled for screen readers:
-{useCustomLength && ( +{useCustomLength && ( + <div role="region" aria-label={t('Custom length selector')}> <CustomUpcomingLength onChange={setTempUpcomingLength} tempValue={tempUpcomingLength} /> + </div> )}
122-132: Add keyboard shortcut for save action.Consider adding keyboard shortcut support:
+import { useHotkeys } from 'react-hotkeys-hook'; +useHotkeys('mod+s', (e) => { + e.preventDefault(); + if (saveActive) { + saveUpcomingLength(); + } +}); <Button isDisabled={!saveActive} onPress={() => { saveUpcomingLength(); close(); }} type="submit" variant="primary" + aria-keyshortcuts="Control+S" > <Trans>Save</Trans> </Button>packages/loot-core/src/shared/schedules.test.ts (1)
347-361: Add edge case tests for getUpcomingDays.Consider adding tests for the following scenarios:
describe('getUpcomingDays', () => { it.each([ ['1', 1], ['7', 7], ['14', 14], ['oneMonth', 32], ['currentMonth', 31], ['2-day', 2], ['5-week', 35], ['3-month', 90], ['4-year', 1460], + ['invalid-unit', 7], // Test invalid unit fallback + ['abc', 7], // Test non-numeric fallback + ['0-day', 1], // Test zero value handling + ['-1-day', 1], // Test negative value handling ])('value of %s returns %i days', (value: string, expected: number) => { expect(getUpcomingDays(value)).toEqual(expected); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4206.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/schedules/CustomUpcomingLength.tsx(1 hunks)packages/desktop-client/src/components/schedules/UpcomingLength.tsx(3 hunks)packages/loot-core/src/shared/schedules.test.ts(2 hunks)packages/loot-core/src/shared/schedules.ts(1 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/schedules/CustomUpcomingLength.tsx (1)
1-10: LGTM! Well-structured props interface and imports.The component's interface is well-defined with clear prop types and necessary imports.
packages/desktop-client/src/components/schedules/CustomUpcomingLength.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/schedules/CustomUpcomingLength.tsx
Show resolved
Hide resolved
|
I cant wait to be able to see the next 20 years of my daily schedule 😆. Looks good so far. Ill let Matt do the review. |
|
This does bring up a potentially real problem that the schedules load after the table so if there is a lot of schedules to go through then it can take a while for them to show. That is probably for a different time since very few people will be using more than a few weeks worth of forward looking. |
|
I like it! Just the one comment I've piggybacked onto CodeRabbit |
We should look to have a loading indication somewhere, although if anyone is trying to have years worth of schedules showing then they should probably expect it to be a little behind. |
…tom-upcoming-length
|
The "up" incrementing arrow in the number input is working for me, but the down isn't decrementing. I think you need a step=1 prop on the Input |
|
Looks like the 1 month and 1 year options are taking you just to the beginning of the next month/year 1 month: 1 year: I can have a look at the code later on but first thought is it's only getting enough days to the end of the period, not to keep the current date within the next period |
packages/desktop-client/src/components/schedules/CustomUpcomingLength.tsx
Outdated
Show resolved
Hide resolved
Add custom option to upcoming length selector.
Not sure I'm happy with the layout of this feature. I welcome any input.
Also the save functionality is a bit abrupt, not sure if a quick save animation is warranted, tho I haven't seen anything like that elsewhere in the application.