-
Notifications
You must be signed in to change notification settings - Fork 19
Refactor colors to use theme tokens #300
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
Refactor colors to use theme tokens #300
Conversation
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Non-Standard Color Scale Increment ▹ view | ||
| Invalid maxHeight value ▹ view | ||
| Unmemoized Color Calculations ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/app/src/components/WalletDetails/styles.ts | ✅ |
| packages/app/src/components/DonorsList/DonorsList.tsx | ✅ |
| packages/app/src/components/WalletDetails/EmptyProfile.tsx | ✅ |
| packages/app/src/components/Layout/Breadcrumb.tsx | ✅ |
| packages/app/src/components/StewardsList/StewardsListItem.tsx | ✅ |
| packages/app/src/theme/theme.ts | ✅ |
| packages/app/src/components/ImpactButton.tsx | ✅ |
| packages/app/src/components/BannerPool.tsx | ✅ |
| packages/app/src/components/ProfileView.tsx | ✅ |
| packages/app/src/components/Dropdown.tsx | ✅ |
| packages/app/src/components/CollectiveHomeCard.tsx | ✅ |
| packages/app/src/components/ActivityLog.tsx | ✅ |
| packages/app/src/components/RowItem.tsx | ✅ |
| packages/app/src/components/FlowingDonationsRowItem.tsx | ✅ |
| packages/app/src/components/DonorsList/DonorsListItem.tsx | ✅ |
| packages/app/src/components/Header/Header.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| goodPurple: { | ||
| 100: '#E2EAFF', | ||
| 200: '#D6E1FF', | ||
| 300: '#CBDAFF', | ||
| 250: '#B8C8F2', | ||
| 400: '#5B7AC6', | ||
| 500: '#2B4483', | ||
| }, |
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.
Non-Standard Color Scale Increment 
Tell me more
What is the issue?
The 'goodPurple' color scale includes a '250' value between 200 and 300, breaking the conventional increment pattern.
Why this matters
Non-standard scale increments can cause confusion and make it difficult to maintain consistent styling patterns across the application.
Suggested change ∙ Feature Preview
Adjust the scale to use standard increments:
goodPurple: {
100: '#E2EAFF',
200: '#D6E1FF',
300: '#CBDAFF',
400: '#5B7AC6',
500: '#2B4483'
}And if the '#B8C8F2' color is needed, consider adding it as a new standard increment or using a different naming convention.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| color: 'goodGrey.400', | ||
| ...InterSmall, | ||
| lineHeight: 24, | ||
| maxHeight: 'auto', |
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.
Invalid maxHeight value 
Tell me more
What is the issue?
The cardDescription style uses 'auto' as a string value for maxHeight, which is not a valid value for this property
Why this matters
Invalid maxHeight value will be ignored by the layout engine, potentially causing text overflow issues
Suggested change ∙ Feature Preview
Either remove the maxHeight property if no limit is needed, or set a specific numeric value:
cardDescription: {
marginTop: 8,
fontSize: 16,
fontWeight: '400',
color: 'goodGrey.400',
...InterSmall,
lineHeight: 24,
overflow: 'hidden',
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| const circleBackgroundColor = | ||
| rank === 1 ? Colors.yellow[100] : rank === 2 ? Colors.gray[700] : rank === 3 ? Colors.orange[400] : 'none'; | ||
| rank === 1 ? 'goodYellow.100' : rank === 2 ? 'goodGrey.700' : rank === 3 ? 'goodOrange.350' : 'none'; | ||
| const circleTextColor = | ||
| rank === 1 ? Colors.yellow[200] : rank === 2 ? Colors.blue[200] : rank === 3 ? Colors.brown[100] : Colors.black; | ||
| rank === 1 ? 'goodYellow.200' : rank === 2 ? 'goodBlue.200' : rank === 3 ? 'goodBrown.100' : 'black'; |
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.
Unmemoized Color Calculations 
Tell me more
What is the issue?
Multiple ternary operations for color selection are recalculated on every render, even though they only depend on the rank prop.
Why this matters
These color calculations could be memoized since they only depend on the rank prop, avoiding unnecessary recalculations during re-renders.
Suggested change ∙ Feature Preview
Use useMemo to cache the color calculations:
const colors = useMemo(() => ({
background: rank === 1 ? 'goodYellow.100' : rank === 2 ? 'goodGrey.700' : rank === 3 ? 'goodOrange.350' : 'none',
text: rank === 1 ? 'goodYellow.200' : rank === 2 ? 'goodBlue.200' : rank === 3 ? 'goodBrown.100' : 'black'
}), [rank]);Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
62b68d9
into
codex/refactor-stylesheet.create-to-native-base-styles
* refactor: migrate more components to native-base styles * fix: agent should run tsc and build * chore(app): add workspace paths to tsconfig (#300)
Summary
as constfor better typingTesting
yarn workspace @gooddollar/goodcollective-app lint --fixyarn workspace @gooddollar/goodcollective-app formatcd packages/app && yarn build:web(fails: missing GoodCollective contract artifacts)yarn test(fails: Failed to load url @gooddollar/goodcollective-contracts/artifacts/...)https://chatgpt.com/codex/tasks/task_b_688381903b108329a142169d3b4052ec
Description by Korbit AI
What change is being made?
Refactor all style objects in various components to be defined with TypeScript's
as constfor improved type safety.Why are these changes being made?
The use of
as constrefines the TypeScript type to literal types based on the actual value of the object, ensuring greater accuracy in type checks and reducing the likelihood of runtime errors due to unintended mutations. This change enhances code reliability and maintainability while aligning with best practices for type safety in TypeScript.