Fix mobile page header buttons #3491
Conversation
WalkthroughThe pull request introduces several modifications across various components in the desktop client. Key changes include the addition of a new utility function for managing hover styles in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (4)
Files selected for processing (9)
Files skipped from review as they are similar to previous changes (9)
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 (
|
✅ 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 No assets were bigger 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
180-182: Approved: Improved mobile interaction and style consistencyThe changes enhance the mobile user experience by switching from a hover effect to a pressed state, which is more appropriate for touch devices. Additionally, the use of
backgroundColorinstead ofbackgroundimproves consistency with React and JavaScript naming conventions.For consistency, consider applying similar changes to other buttons in the component if they haven't been updated yet. This will ensure a uniform interaction model across the mobile interface.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (1)
130-152: Great improvement to user interaction and accessibility.The replacement of the
Textcomponent with aButtoncomponent significantly enhances the user experience and accessibility of the account name element. The use of glamor for styling and theonPressprop are well-suited for mobile interactions.A minor suggestion for improvement:
Consider extracting the inline styles for the
Textcomponent (lines 143-148) into a separate style object or using a styled component for better maintainability. For example:const accountNameTextStyle = { fontSize: 17, fontWeight: 500, ...styles.underlinedText, ...styles.lineClamp(2), }; // Then in the JSX: <Text style={accountNameTextStyle}> {`${account.closed ? 'Closed: ' : ''}${account.name}`} </Text>This change would improve code readability and make it easier to maintain or update the text styles in the future.
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (2)
788-791: Consider enhancing the pressed state visual feedback.The removal of
noTapHighlightand addition ofdata-pressedstate improves consistency. However, the current implementation uses a transparent background for the pressed state, which might not provide sufficient visual feedback to users.Consider adding a subtle background color or other visual indicator for the pressed state to improve user experience. For example:
'&[data-pressed]': { - backgroundColor: 'transparent', + backgroundColor: theme.buttonBackgroundHover, },
1938-1960: Enhanced month selection with improved interactionThe replacement of the Text component with a Button component improves accessibility and user interaction. The styling is consistent with other buttons in the component, and the underlined text maintains the visual cue for interaction.
Consider adding an aria-label to the button to improve accessibility further. For example:
<Button variant="bare" + aria-label={`Select month: ${monthUtils.format(month, 'MMMM yyyy')}`} className={String( // ... (existing code) )} onPress={() => { onOpenMonthMenu?.(month); }} > // ... (existing code) </Button>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- packages/desktop-client/src/components/common/Button2.tsx (3 hunks)
- packages/desktop-client/src/components/mobile/MobileBackButton.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (5 hunks)
- packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx
Additional comments not posted (11)
packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx (1)
31-33: Improved button interaction and stylingThe changes to the button styling are well-implemented and align with the PR objectives:
- Replacing
:hoverwith&[data-pressed]improves the mobile user experience by providing appropriate feedback for touch interactions.- Changing
backgroundtobackgroundColoris more specific and follows CSS best practices.- The use of theme variables (
theme.mobileHeaderTextandtheme.mobileHeaderTextHover) maintains consistency in the design system.These updates enhance both the functionality and maintainability of the component.
packages/desktop-client/src/components/mobile/MobileBackButton.tsx (1)
29-31: Improved mobile interaction and CSS namingThese changes enhance the button's behavior and styling:
- Switching from
data-hoveredtodata-pressedimproves the mobile user experience, as it's more relevant for touch interfaces.- Updating
backgroundtobackgroundColoradheres to the camelCase convention, improving code consistency.These modifications align well with the PR objective to fix button2 styles and enhance overall code quality.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (2)
10-10: LGTM: New imports support the component changes.The addition of
cssfrom 'glamor' andButtonfrom '../../common/Button2' aligns well with the modifications made to theAccountNamecomponent, supporting the shift from aTextcomponent to a styledButtoncomponent.Also applies to: 36-36
Line range hint
1-290: Summary: Positive enhancement to mobile user experience.The changes in this file, particularly to the
AccountNamecomponent, represent a significant improvement in the mobile user experience. By converting the account name from a simple text element to an interactive button, the code now provides better accessibility and clearer user interaction cues. The use of glamor for styling and the implementation of a pressed state further enhance the mobile-friendly nature of the component.These modifications align well with modern React and mobile development best practices, improving both the functionality and the maintainability of the code. The overall impact of these changes is positive, contributing to a more robust and user-friendly mobile interface for account transactions.
packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (3)
983-986: Consistent styling with ExpenseGroupHeaderThe changes here mirror those in the ExpenseGroupHeader component, maintaining consistency in the UI.
Please refer to the previous comment about enhancing the pressed state visual feedback, as the same suggestion applies here.
1929-1932: Improved user feedback for button interactionThe addition of pressed state styles enhances the user experience by providing clear visual feedback. The use of theme variables ensures consistency with the overall design.
1973-1976: Consistent styling with previous month buttonThe changes here mirror those made to the previous month button, maintaining consistency in the UI and user interaction. The use of theme variables ensures a cohesive design.
packages/desktop-client/src/components/common/Button2.tsx (4)
3-3: ImportinguseCallbackThe addition of
useCallbackto the imports is appropriate for optimizing performance through memoization.
108-113: Refactoring hover styles into_getHoveredStylesEncapsulating hover styles within the
_getHoveredStylesfunction enhances code readability and reusability.
157-185: Memoization ofdefaultButtonClassNamewithuseCallbackUsing
useCallbackto memoizedefaultButtonClassNameis effective. The dependency array[Icon, bounce, variant, variantWithDisabled]correctly includes all external variables used within the callback.
187-198: Memoization ofbuttonClassNamewithuseCallbackThe
useCallbackhook forbuttonClassNamecorrectly optimizes the function by memoizing it based on theclassNameprop. This prevents unnecessary recalculations whenclassNameremains unchanged.
| '&[data-pressed]': { | ||
| backgroundColor: 'transparent', | ||
| }, |
There was a problem hiding this comment.
Approved: Improved interaction model for button styles
The change from hover to pressed state is a good improvement, especially for mobile devices. This aligns well with the PR objective of fixing button2 styles.
Consider adding a subtle transition effect for smoother visual feedback:
'&[data-pressed]': {
backgroundColor: 'transparent',
+ transition: 'background-color 0.2s ease',
},This will create a smooth fade effect when the button is pressed, enhancing the user experience.
For better accessibility, consider adding an aria-pressed attribute to the button that changes with the pressed state. This will improve the experience for users relying on screen readers.
Committable suggestion was skipped due to low confidence.
| {Icon && ( | ||
| <Icon style={{ height: 15, paddingLeft: 5, paddingRight: 3 }} /> | ||
| )} |
There was a problem hiding this comment.
❓ question: intentionally removing this? If so: might as well remove the Icon prop.
There was a problem hiding this comment.
I think we should just remove this Icon since we can just pass the Icon as part of the children prop:
<Button>
<Icon />
....
</Button>There was a problem hiding this comment.
We can definitely remove it now since it's not used.
The main reason for adding it as a prop instead of passing in children was the visual appearance. The icon didn't really look nice because of the large left margin the button automatically applies.
|
@CodeRabbit review |
Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/Page.tsx (1)
60-66: Improved styling approach for mobile header buttonsThe changes effectively address the PR objective of fixing mobile page header buttons. The new styling approach using CSS-in-JS syntax for child elements improves maintainability and separation of concerns. The addition of a pressed state for buttons enhances user feedback, which is crucial for mobile interfaces.
Consider extracting the styles into a separate constant or using a CSS-in-JS library like styled-components for better organization. For example:
const mobileHeaderStyles = { backgroundColor: theme.mobileHeaderBackground, '& *': { color: theme.mobileHeaderText, }, '& button[data-pressed]': { backgroundColor: theme.mobileHeaderTextHover, }, }; // Then in the component: <View style={{ ...mobileHeaderStyles, ...style }}>This approach would make the component more readable and easier to maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
packages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-loads-the-budget-page-with-budgeted-amounts-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-loads-the-budget-page-with-budgeted-amounts-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-loads-the-budget-page-with-budgeted-amounts-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3491.mdis excluded by!**/*.md
Files selected for processing (7)
- packages/desktop-client/src/components/Page.tsx (1 hunks)
- packages/desktop-client/src/components/common/Button2.tsx (3 hunks)
- packages/desktop-client/src/components/mobile/MobileBackButton.tsx (2 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (2 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx (4 hunks)
- packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- packages/desktop-client/src/components/common/Button2.tsx
- packages/desktop-client/src/components/mobile/MobileBackButton.tsx
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx
- packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx
joel-jeremy
force-pushed
the
button2-fix-styles
branch
from
ad8e27e to
fd54c52
Compare
No description provided.