♻️ moving more components to the new component-lib pkg #4346
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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
MatissJanis
changed the title
WalkthroughThe changes update import paths and re-export declarations across the project. In the ESLint configuration, the source for the Menu component has been changed to point to the centralized component library. In the component-library, several new export paths have been introduced in the package.json file, making various UI components (such as AlignedText, Block, Card, and others) accessible from their new locations. Additionally, many internal import statements have been updated to reference local style and theme files with new relative paths. In the desktop-client package, components like Menu, Block, Card, and others are now re-exported with deprecation notices, directing users to import them directly from the new centralized component library. These modifications consolidate the component structure without altering component functionality. Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (1)
packages/component-library/src/InitialFocus.ts (1)
18-30: Consider adding cleanup for the setTimeout.The timeout should be cleared when the component unmounts to prevent potential memory leaks.
useEffect(() => { + let timeoutId: NodeJS.Timeout; if (node.current) { - setTimeout(() => { + timeoutId = setTimeout(() => { if (node.current) { node.current.focus(); node.current.setSelectionRange(0, 10000); } }, 0); } + return () => { + if (timeoutId) { + clearTimeout(timeoutId); + } + }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4346.mdis excluded by!**/*.md
📒 Files selected for processing (33)
eslint.config.mjs(1 hunks)packages/component-library/package.json(1 hunks)packages/component-library/src/Block.tsx(1 hunks)packages/component-library/src/Card.tsx(1 hunks)packages/component-library/src/InitialFocus.ts(1 hunks)packages/component-library/src/InlineField.tsx(1 hunks)packages/component-library/src/Label.tsx(1 hunks)packages/component-library/src/Menu.tsx(1 hunks)packages/component-library/src/Paragraph.tsx(1 hunks)packages/component-library/src/Popover.tsx(1 hunks)packages/component-library/src/Text.tsx(1 hunks)packages/component-library/src/Toggle.tsx(1 hunks)packages/component-library/src/Tooltip.tsx(1 hunks)packages/desktop-client/src/components/common/AlignedText.ts(1 hunks)packages/desktop-client/src/components/common/Block.ts(1 hunks)packages/desktop-client/src/components/common/Button.tsx(1 hunks)packages/desktop-client/src/components/common/Card.ts(1 hunks)packages/desktop-client/src/components/common/FormError.ts(1 hunks)packages/desktop-client/src/components/common/InitialFocus.ts(1 hunks)packages/desktop-client/src/components/common/InlineField.ts(1 hunks)packages/desktop-client/src/components/common/Label.ts(1 hunks)packages/desktop-client/src/components/common/Menu.ts(1 hunks)packages/desktop-client/src/components/common/Modal.tsx(1 hunks)packages/desktop-client/src/components/common/Paragraph.ts(1 hunks)packages/desktop-client/src/components/common/Popover.ts(1 hunks)packages/desktop-client/src/components/common/SpaceBetween.ts(1 hunks)packages/desktop-client/src/components/common/Stack.ts(1 hunks)packages/desktop-client/src/components/common/Text.ts(1 hunks)packages/desktop-client/src/components/common/TextOneLine.ts(1 hunks)packages/desktop-client/src/components/common/Toggle.ts(1 hunks)packages/desktop-client/src/components/common/Tooltip.ts(1 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(2 hunks)packages/desktop-client/src/components/table.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (26)
- packages/component-library/src/Popover.tsx
- packages/desktop-client/src/components/common/AlignedText.ts
- packages/desktop-client/src/components/common/FormError.ts
- packages/desktop-client/src/components/common/Card.ts
- packages/component-library/src/Block.tsx
- packages/desktop-client/src/components/common/Block.ts
- packages/desktop-client/src/components/common/Label.ts
- packages/component-library/src/Menu.tsx
- packages/desktop-client/src/components/common/Stack.ts
- packages/desktop-client/src/components/common/TextOneLine.ts
- packages/desktop-client/src/components/common/Menu.ts
- packages/desktop-client/src/components/common/Popover.ts
- packages/desktop-client/src/components/common/Button.tsx
- packages/desktop-client/src/components/common/InlineField.ts
- packages/component-library/src/Toggle.tsx
- packages/desktop-client/src/components/common/Tooltip.ts
- packages/component-library/src/Card.tsx
- packages/desktop-client/src/components/common/Paragraph.ts
- packages/desktop-client/src/components/common/Text.ts
- eslint.config.mjs
- packages/component-library/src/Tooltip.tsx
- packages/component-library/src/InlineField.tsx
- packages/component-library/src/Text.tsx
- packages/component-library/src/Paragraph.tsx
- packages/component-library/src/Label.tsx
- packages/desktop-client/src/components/common/Toggle.ts
🔇 Additional comments (8)
packages/desktop-client/src/components/common/SpaceBetween.ts (2)
1-1: LGTM! Clean import with clear alias naming.The import statement correctly references the new component library location, following the project's component migration initiative.
3-4: LGTM! Well-documented deprecation with clear migration path.The deprecation notice and re-export provide a smooth transition path for users while maintaining backward compatibility. The JSDoc comment clearly guides developers to the new import location.
packages/desktop-client/src/components/common/InitialFocus.ts (1)
1-4: LGTM! Clean re-export with proper deprecation notice.The component has been correctly moved to the component library and re-exported with a clear deprecation notice guiding users to the new import path.
packages/component-library/src/InitialFocus.ts (1)
1-36: LGTM! Clean and well-structured implementation.The component is well-implemented with:
- Clear type definitions
- Proper use of React hooks
- Flexible children prop handling
- Good interaction with ScopeTab
packages/desktop-client/src/components/common/Modal.tsx (1)
19-26: LGTM! Clean import path updates.Import paths have been correctly updated to use components from the new component library.
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (1)
12-17: LGTM! Clean import path updates.Import paths have been correctly updated to use components from the new component library.
packages/desktop-client/src/components/table.tsx (1)
20-27: LGTM! Import paths updated to use the new component library.The changes correctly migrate the imports from local paths to the new centralized component library, which aligns with the PR objectives of moving components to the component-lib package.
packages/component-library/package.json (1)
18-38: LGTM! Export paths added for new components in the library.The changes correctly add export paths for multiple components, following a consistent pattern and supporting the migration of components to the centralized library.
|
@coderabbitai can you open a new PR that updates all deprecated Button2 imports with the new |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@MatissJanis: I'll create an issue to track this migration task. 🏁 Script executed: Length of output: 54 🏁 Script executed: Length of output: 622 @MatissJanis: I've created issue #4355 to track this migration. Let me search for all Button2 imports to ensure we have a complete list of files that need to be updated. 🏁 Script executed: Length of output: 9405 @MatissJanis: I've created issue #4355 to track the Button2 migration. Let me refine the search to get an accurate list of files that need to be updated. I'll update the issue once I have the complete list of files with Button2 imports. 🏁 Script executed: Length of output: 369 🏁 Script executed: Length of output: 16322 @MatissJanis: I've updated issue #4355 with a complete list of files that need to be migrated. There are over 100 files that need to be updated to use the new Would you like me to help break this down into smaller, more manageable PRs to make the migration easier to review and test? 🏁 Script executed: Length of output: 815 @MatissJanis: I've updated issue #4355 with a complete list of files that need to be migrated. There are over 100 files that need to be updated to use the new Given the large number of files, would you like me to suggest a strategy for breaking this down into smaller, more manageable PRs? For example, we could:
What approach would you prefer? 🏁 Script executed: Length of output: 20173 @MatissJanis: I've updated issue #4355 with all 137 files that need to be migrated. Looking at the files, I suggest breaking this down into multiple PRs by directory/feature area. Here's a suggested breakdown:
This way:
Would you like me to create separate issues for each of these groups to track the migration? ✏️ Learnings added
|
|
@coderabbitai can you open a single PR that does all of this work? No need to break it into smaller PRs |
|
Tip For best results, initiate chat on the files or code changes. @MatissJanis I apologize, but I am not able to create PRs directly. However, I've already gathered all the necessary information in issue #4355 which you can use to create the PR manually. The issue contains:
Would you like me to provide any additional information to help you create this PR? |
Following up to: #4169