Fixes #3536 Fixes bank sync notification overlap with uncategorized transactions #3578
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/BankSyncStatus.tsx (1)
35-37: Improved layout handling addresses overlap issueThe new styling properties effectively address the notification overlap problem:
flexDirection: 'row'ensures horizontal layout, suitable for a status bar.overflow: 'hidden'prevents content from spilling out, directly addressing the overlap issue.textWrap: 'nowrap'maintains a compact, single-line layout.These changes, combined with the removal of absolute positioning (as per the AI summary), create a more flexible and robust layout that should resolve the overlap problem mentioned in the PR objectives.
Consider adding a
maxWidthproperty to ensure the component doesn't grow too wide on large screens. This could help maintain a balanced layout across different screen sizes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3577.mdis excluded by!**/*.md
📒 Files selected for processing (3)
- packages/desktop-client/src/components/BankSyncStatus.tsx (1 hunks)
- packages/desktop-client/src/components/FinancesApp.tsx (0 hunks)
- packages/desktop-client/src/components/Titlebar.tsx (2 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/FinancesApp.tsx
🔇 Additional comments (6)
packages/desktop-client/src/components/BankSyncStatus.tsx (2)
24-26: Simplified animation improves layout stabilityThe removal of the
translateYproperty from the animation configuration is a good change. This simplification:
- Aligns with the PR objective of preventing overlap with other notifications.
- Improves layout stability by eliminating vertical movement during transitions.
- May slightly enhance performance due to simpler animations.
These changes contribute to resolving the issue of overlapping notifications mentioned in the PR objectives.
24-37: Overall improvements address PR objectives, but clarification neededThe changes effectively address the main PR objective of fixing the notification overlap:
- Simplified animations reduce vertical movement.
- Improved layout handling with
overflow: 'hidden'and horizontal alignment.- Minimal, targeted changes reduce the risk of unintended side effects.
However, the PR description mentions a loss of centered position at full resolution, which isn't directly addressed in these code changes.
Could you please clarify how the centered position is affected? If this is an intentional trade-off, it might be worth documenting in a comment. If not, we may need to explore additional styling options to maintain centering while preventing overlap.
To verify the impact on different screen sizes, consider running the following command:
This will help us identify if there are any existing responsive design considerations that might be affected by these changes.
packages/desktop-client/src/components/Titlebar.tsx (4)
29-29: LGTM: Import statement for BankSyncStatusThe import statement for
BankSyncStatusis correctly added and follows the existing import style in the file.
336-336: Verify layout and alignment across different screen sizesThe addition of
BankSyncStatusshould resolve the overlap issue. However, please ensure the following:
- Verify that the layout works well on different screen sizes, especially smaller ones.
- Confirm that the notification div (presumably part of
BankSyncStatus) aligns with other buttons as mentioned in the PR objectives.- Test the responsiveness of the Titlebar with the new component added.
To assist in verifying the layout and responsiveness, consider adding snapshot tests or visual regression tests for the Titlebar component with various screen sizes. Additionally, manual testing on different devices and screen sizes is recommended.
Line range hint
1-345: Summary of Titlebar.tsx changesThe changes to the Titlebar component appear to address the PR objectives by adding the
BankSyncStatuscomponent. Here's a summary of the review:
- The import and usage of
BankSyncStatusare correctly implemented.- The placement of
BankSyncStatusshould resolve the overlap issue with the uncategorized transactions notification.- Verification is needed for:
- Conditional rendering of
BankSyncStatus(if necessary)- Any required props for
BankSyncStatus- Layout and alignment across different screen sizes
- Responsiveness of the Titlebar with the new component
Overall, the changes look good, but please address the verification points to ensure the implementation fully meets the requirements and maintains a good user experience across all devices.
336-336: Verify BankSyncStatus rendering conditions and propsThe
BankSyncStatuscomponent is correctly placed beforeUncategorizedButton, which should address the overlap issue mentioned in the PR objectives. However, please consider the following points:
- The component is rendered unconditionally. Verify if this is the intended behavior or if it should be conditionally rendered based on certain criteria.
- No props are passed to
BankSyncStatus. Ensure that the component is designed to work without props or if any necessary props need to be added.To help verify the
BankSyncStatuscomponent's implementation, you can run the following script:
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
|
The-Firexx
changed the title
|
Wouldn't this change remove the bank sync on the mobile view? |
This pull request is to avoid the bank sync notification overlap with uncategorized transactions.
This fixes #3536
Visually it will now look like this. The "Bank sync notification" will start to overflow as hidden.
I also changed the position of the div to stay in the same div as the rest of the buttons, losing is "center position" when the window is full resolution.
If the solution is not ideal, please close this pull request.
Thank you.