✨ component library - minimalistic infrastructure #4169
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
WalkthroughThis pull request introduces a new component library package Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
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: 5
🧹 Nitpick comments (10)
packages/component-library/src/icons/AnimatedLoading.tsx (2)
7-10: Clarify the rotation degrees in keyframesThe rotation animation goes from
-90degto666deg. Using666degis unconventional and might be confusing for future maintainers. Typically, a full rotation is360deg.Consider adjusting the keyframes to use full rotations for clarity:
'0%': { transform: 'rotate(-90deg)' }, -'100%': { transform: 'rotate(666deg)' }, +'100%': { transform: 'rotate(270deg)' },This change rotates from
-90degto270deg, resulting in a full 360-degree rotation.
15-21: Enhance animation performance withwill-changeAdding the
will-change: transformproperty can hint to the browser to optimize the rendering of the animated element.Apply this diff to include the
will-changeproperty:animationIterationCount: 'infinite', lineHeight: 0, +willChange: 'transform',packages/component-library/src/icons/Loading.tsx (1)
4-5: Optimize gradient ID generation to prevent unnecessary re-renders.The current implementation generates a new gradient ID on every component mount. Consider using
useMemoto optimize this:- const [gradientId] = useState('gradient-' + Math.random()); + const gradientId = useMemo(() => 'gradient-' + Math.random(), []);packages/desktop-client/src/style/styles.ts (1)
8-9: Consider adding migration guide in deprecation notice.The deprecation notice should include guidance on how to migrate:
-/** @deprecated please import styles from '@actual-app/components/styles' */ +/** + * @deprecated Please import styles from '@actual-app/components/styles' + * Migration guide: + * 1. Update your imports to use '@actual-app/components/styles' + * 2. Replace any local style overrides with theme tokens + * @example + * import { styles } from '@actual-app/components/styles'; + */packages/component-library/src/styles.ts (1)
63-69: Consolidate media queries using a helper function.Multiple media queries with similar breakpoints could be consolidated:
+const mediaQuery = (breakpoint: string, styles: CSSProperties) => ({ + [`@media (min-width: ${breakpoint})`]: styles, +}); page: { flex: 1, - '@media (max-height: 550px)': { - minHeight: 700, - }, - [`@media (min-width: ${tokens.breakpoint_small})`]: { - paddingTop: 36, - }, + ...mediaQuery('(max-height: 550px)', { minHeight: 700 }), + ...mediaQuery(tokens.breakpoint_small, { paddingTop: 36 }), },Also applies to: 74-77, 81-83
packages/desktop-client/src/components/FatalError.tsx (1)
Line range hint
18-24: Consider extracting error type definitions.The error type definitions could be moved to a separate types file for better maintainability:
+// New file: types/errors.ts +export type AppError = Error & { + type?: string; + IDBFailure?: boolean; + SharedArrayBufferMissing?: boolean; + BackendInitFailure?: boolean; +}; -type AppError = Error & { - type?: string; - IDBFailure?: boolean; - SharedArrayBufferMissing?: boolean; - BackendInitFailure?: boolean; -};packages/component-library/src/theme.ts (2)
1-203: Consider organizing theme variables into logical groupings for better maintainability.The theme object contains a comprehensive set of CSS variables, but organizing them into nested objects by component type would improve maintainability and make it easier to extend the theme system in the future.
Consider restructuring the theme object like this:
export const theme = { - pageBackground: 'var(--color-pageBackground)', + page: { + background: 'var(--color-pageBackground)', + text: 'var(--color-pageText)', + // ... other page-related variables + }, + button: { + primary: { + text: 'var(--color-buttonPrimaryText)', + // ... other primary button variables + }, + normal: { + // ... normal button variables + }, + // ... other button types + }, // ... other components };
1-203: Consider adding TypeScript types for better type safety.The theme object would benefit from TypeScript type definitions to ensure type safety when accessing theme values throughout the application.
Consider adding type definitions:
type ThemeColor = string; interface ThemeColors { pageBackground: ThemeColor; pageText: ThemeColor; // ... other color definitions } export const theme: ThemeColors = { // ... existing theme object };packages/component-library/package.json (2)
2-4: Consider using 0.1.0 as the initial version.Following SemVer conventions, 0.1.0 is typically used for initial development releases rather than 0.0.1, especially for packages intended for public release.
{ "name": "@actual-app/components", - "version": "0.0.1", + "version": "0.1.0", "license": "MIT",
8-11: Lock the react-aria-components version.Since this is a new package, it's better to lock the dependency version to avoid potential breaking changes.
"dependencies": { "@emotion/css": "^11.13.4", - "react-aria-components": "^1.4.1" + "react-aria-components": "1.4.1" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4169.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
packages/component-library/package.json(1 hunks)packages/component-library/src/Button.tsx(1 hunks)packages/component-library/src/View.tsx(1 hunks)packages/component-library/src/icons/AnimatedLoading.tsx(1 hunks)packages/component-library/src/icons/Loading.tsx(1 hunks)packages/component-library/src/styles.ts(1 hunks)packages/component-library/src/theme.ts(1 hunks)packages/component-library/src/tokens.ts(1 hunks)packages/desktop-client/package.json(1 hunks)packages/desktop-client/src/components/FatalError.tsx(1 hunks)packages/desktop-client/src/components/common/Button.tsx(1 hunks)packages/desktop-client/src/components/common/Button2.ts(1 hunks)packages/desktop-client/src/components/common/View.ts(1 hunks)packages/desktop-client/src/icons/AnimatedLoading.tsx(1 hunks)packages/desktop-client/src/style/styles.ts(1 hunks)packages/desktop-client/src/style/theme.tsx(1 hunks)packages/desktop-client/src/tokens.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/desktop-client/src/components/common/Button2.ts
- packages/desktop-client/src/components/common/View.ts
- packages/desktop-client/src/icons/AnimatedLoading.tsx
- packages/desktop-client/src/components/common/Button.tsx
- packages/component-library/src/Button.tsx
- packages/component-library/src/View.tsx
- packages/desktop-client/src/tokens.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/component-library/src/tokens.ts
[error] 31-31: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (4)
packages/desktop-client/src/style/theme.tsx (1)
99-99: Verify compatibility with centralized theme exportsBy re-exporting everything from
@actual-app/components/theme, ensure that any locally defined theme properties or overrides previously present intheme.tsxare accounted for in the new centralized theme. This change might affect components that relied on local theme customizations.Do you want me to scan the codebase to identify any components that might be impacted by this change?
packages/desktop-client/src/style/styles.ts (1)
Line range hint
13-71: Verify scrollbar detection impact across platforms.The scrollbar detection logic is platform-specific. Ensure this works correctly with the new centralized styles.
✅ Verification successful
Scrollbar detection implementation is properly centralized and platform-aware
The scrollbar detection logic is correctly implemented with a clean separation of concerns:
- Base style structure defined in component-library
- Platform-specific detection and styling centralized in desktop-client
- No conflicting implementations found across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for platform-specific style overrides rg -l "scrollbar" --type ts --type tsxLength of output: 66
Script:
#!/bin/bash # Search for scrollbar-related code in .ts files rg "scrollbar" --type ts -l # Search for Platform.env usage rg "Platform\.env" --type ts -l # Search for imports of styles.ts rg "from.*style.?/styles" --type ts -lLength of output: 1483
Script:
#!/bin/bash # Check component-library scrollbar implementation cat packages/component-library/src/styles.ts # Search for other platform checks that might affect scrollbars rg -g "*.ts" -g "*.tsx" "(Platform|platform).*(\bweb\b|\bdesktop\b|\bmobile\b)" -lLength of output: 3671
packages/component-library/src/styles.ts (1)
100-100: Add documentation for the commented line-height TODO.The TODO comment should be addressed or documented properly:
packages/desktop-client/src/components/FatalError.tsx (1)
4-5: Verify all component imports are migrated consistently.Check if other components like
Block,Link, etc., should also be migrated to the component library.
There was a problem hiding this comment.
Code changes look good. Wondering how you're thinking about backward compatibility in the future if we want to do a breaking change in the interface of a component? I feel like we should codify something to protect plugin users a bit, e.g. "Announce breaking changes one release before they are implemented"
Creating a very minimalistic infrastructure for the shared component library. The intention is to eventually publish this to NPM as a package. Next - this package can be consumed in plugins to build beautiful UI that matches the UX of Actual.
More common components would need to be moved to this plugin before we open it up though. But I'll slowly work on moving everything over and updating import paths.