Update Sidebar UI #3457
Update Sidebar UI #3457
Conversation
Moved "More->Payees" and "More->Rules" into menu Rearranged Menu Items Anchored Budget, Reports, and Schedule above account list. Anchored Add Account below account list Now, only accounts scroll, everything else is in a fixed position.
Removed the word "More" and changed the layout of the extra features (scheduled, payees, rules) Updated layout and file structure Removed Edit Budget Name from menu and placed pencil Icon next to budget name. Todo: Hide edit budget name icon when not hovering over budget name.
Moved code from Tools into ActionButtons. Both ActionButtons and BottomButtons now use an array to map buttons. To Do: Move array of buttons to sidebar.
ActionButtons and BottomButtons are now empty and reusable containers.
For budget and Off Budget account groups can expand and contract. Moved "All accounts" outside of scrollable area.
For Budget and Off Budget account groups can now collapse
Removed code that was commented out.
Added key prop to ActionItems and BottomsButtoms to remove console warning.
Fixed new budget creation error in collapsible account groups.
✅ 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
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
|
Added Release Notes MD file
|
Started this PR in response to request from comment on #3414. Added Release Notes, but it is not showing in the checks. |
WalkthroughThe changes in this pull request involve the introduction of the Changes
Possibly related PRs
Suggested labels
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: 3
Outside diff range and nitpick comments (6)
upcoming-release-notes/3457.md (1)
6-11: The release notes content looks good!The body section provides a clear and concise overview of the key changes made to the sidebar UI. It aligns well with the PR objectives and effectively communicates the enhancements to the user experience.
Consider making the following minor grammatical improvements based on the static analysis hints:
- Line 10: Add "the" before "menu" and "a" before "pencil icon".
- Line 11: Add "the" before "scrollable area".
Here's a suggested diff:
-Removed "Rename budget" from menu and added pencil icon next to budget name. +Removed "Rename budget" from the menu and added a pencil icon next to the budget name. -Changed scrollable area to just accounts. Action Buttons and All accounts do not scroll. +Changed the scrollable area to just accounts. Action Buttons and All accounts do not scroll.These changes are not critical but can help improve the overall readability and grammatical correctness of the release notes.
Tools
LanguageTool
[uncategorized] ~10-~10: You might be missing the article “the” here.
Context: ...al estate. Removed "Rename budget" from menu and added pencil icon next to budget na...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~10-~10: You might be missing the article “a” here.
Context: ...ved "Rename budget" from menu and added pencil icon next to budget name. Changed scrol...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~10-~10: You might be missing the article “the” here.
Context: ...from menu and added pencil icon next to budget name. Changed scrollable area to just a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/desktop-client/src/components/sidebar/BottomButtons.tsx (1)
29-35: Consider extracting inline styles to a separate stylesheet.The inline styles for the
Viewcomponent could be extracted to a separate stylesheet. This would improve the maintainability and readability of the component.packages/desktop-client/src/components/sidebar/ActionButtons.tsx (2)
40-47: JSX structure and conditional rendering look good!The JSX is correctly structured, and the conditional rendering of
Itemcomponents based onhidableandisOpenis properly implemented.Consider extracting the inline style on the
Viewcomponent into a separate style object for better readability and maintainability. For example:+ const styles = { + container: { + padding: '10px 0', + flexShrink: 0, + }, + }; + return ( - <View style={{ padding: '10px 0', flexShrink: 0}}> + <View style={styles.container}> {/* ... */} </View> );
48-55: SecondaryItem rendering looks good, but reconsider the empty title prop.The
SecondaryItemcomponent is correctly rendered with the appropriate props, and the conditional rendering of theIconbased on theisOpenstate is a good practice.Consider providing a more meaningful label for the
titleprop instead of an empty string. A descriptive label can improve accessibility and clarity for users. If the functionality is clear without a label, consider using an icon-only button.packages/desktop-client/src/components/sidebar/Accounts.tsx (1)
17-21: Consider improving the type definition forAccountsProps.The static analysis hints suggest the following improvements:
Avoid using an empty object pattern (
{}) for theAccountsPropstype. Instead, explicitly define the object shape or use a more specific type.Avoid using
{}as a type as it means "any non-nullable value". Prefer explicitly defining the object shape or using a more specific type.Here's a suggestion to improve the type definition:
-type AccountsProps = { -}; +type AccountsProps = Record<string, never>;This explicitly defines
AccountsPropsas an object type with no properties.Tools
Biome
[error] 20-21: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 17-18: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/desktop-client/src/components/sidebar/AccountGroup.tsx (1)
71-77: Consider extracting inline styles for better maintainability.The inline styles defined in the
accountGroupStyleobject may make it harder to maintain and reuse styles across components.Consider extracting the inline styles into a separate styles object or using a CSS-in-JS library for better maintainability and reusability of styles.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- packages/desktop-client/src/components/sidebar/Account.tsx (0 hunks)
- packages/desktop-client/src/components/sidebar/AccountGroup.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/AccountGroupName.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Accounts.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/ActionButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/BottomButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (6 hunks)
- packages/desktop-client/src/components/sidebar/Tools.tsx (0 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
- upcoming-release-notes/3457.md (1 hunks)
Files not reviewed due to no reviewable changes (2)
- packages/desktop-client/src/components/sidebar/Account.tsx
- packages/desktop-client/src/components/sidebar/Tools.tsx
Additional context used
Learnings (1)
packages/loot-core/src/types/prefs.d.ts (1)
Learnt from: MatissJanis PR: actualbudget/actual#3458 File: packages/loot-core/src/client/state-types/prefs.d.ts:5-5 Timestamp: 2024-09-17T20:04:47.663Z Learning: In future reviews, ensure that changes related to `PrefsState` in `prefs.d.ts` do not incorrectly assume necessary updates in other parts of the codebase. Verify the impact thoroughly before making suggestions.
LanguageTool
upcoming-release-notes/3457.md
[uncategorized] ~10-~10: You might be missing the article “the” here.
Context: ...al estate. Removed "Rename budget" from menu and added pencil icon next to budget na...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~10-~10: You might be missing the article “a” here.
Context: ...ved "Rename budget" from menu and added pencil icon next to budget name. Changed scrol...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~10-~10: You might be missing the article “the” here.
Context: ...from menu and added pencil icon next to budget name. Changed scrollable area to just a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
Biome
packages/loot-core/src/types/prefs.d.ts
[error] 62-62: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
packages/desktop-client/src/components/sidebar/Accounts.tsx
[error] 20-21: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 17-18: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
[error] 55-55: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 86-86: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 96-96: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 97-97: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
GitHub Check: lint
packages/loot-core/src/types/prefs.d.ts
[failure] 62-62:
Prefer using the primitiveobjectas a type name, rather than the upper-casedObject
Additional comments not posted (19)
upcoming-release-notes/3457.md (1)
1-4: LGTM!The header section is well-structured and contains appropriate metadata for the release notes.
packages/desktop-client/src/components/sidebar/BottomButtons.tsx (1)
23-41: LGTM!The
BottomButtonscomponent implementation looks good. It correctly renders the buttons based on the providedbuttonsprop and uses theSecondaryItemcomponent for each button.packages/desktop-client/src/components/sidebar/ActionButtons.tsx (3)
1-19: LGTM!The import statements are correctly defined and necessary for the component.
20-31: Great job defining the TypeScript types!The
ActionButtonItemsandActionButtonsPropstypes provide a clear contract for the component's props and allow for flexibility in theIconproperty.
33-39: Excellent use of React hooks!The component correctly uses the
useTranslation,useState, anduseCallbackhooks to manage internationalization, state, and memoization.packages/loot-core/src/types/prefs.d.ts (1)
62-62: Use a more specific type or the primitiveobjectinstead ofObject.The static analysis hints correctly flag the usage of
Objectas a type. Using a broad type likeObjectmay lead to type-related issues and make the code harder to understand and maintain.Consider defining a more specific type that represents the structure of the
'ui.collapsedAccountGroups'object or use the primitiveobjectinstead.-'ui.collapsedAccountGroups': Object; +'ui.collapsedAccountGroups': Record<string, boolean>;Also, verify the impact of this change on other parts of the codebase that rely on the
LocalPrefstype and ensure necessary updates are made.Tools
Biome
[error] 62-62: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
GitHub Check: lint
[failure] 62-62:
Prefer using the primitiveobjectas a type name, rather than the upper-casedObjectpackages/desktop-client/src/components/sidebar/Accounts.tsx (6)
2-4: LGTM!The imports are correctly used and necessary for the component.
7-7: LGTM!The import is correctly used and necessary for the
onReorderfunction.
14-15: LGTM!The imports are correctly used and necessary for the refactored component structure.
23-24: LGTM!The
useDispatchanduseAccountshooks are correctly used and necessary for the component.
29-40: LGTM!The refactored
onReorderfunction is more streamlined and correctly dispatches themoveAccountaction.
45-80: LGTM!The refactored structure using the
AccountGroupandAccountGroupNamecomponents improves readability and maintainability. The conditional rendering of account groups ensures that only non-empty groups are displayed.packages/desktop-client/src/components/sidebar/AccountGroupName.tsx (1)
1-145: LGTM!The
AccountGroupNamecomponent is well-structured, follows best practices, and enhances the sidebar functionality with its clear and interactive design. The use of TypeScript for prop types ensures type safety, and the modular design promotes maintainability and reusability.The component's layout is nicely structured using nested
Viewcomponents, allowing for flexible styling and alignment. The conditional rendering of theSvgExpandArrowicon based on the presence of thetoggleAccountsfunction is a good approach. The link styling reflects the account group's status effectively, with dynamic styles applied based on theupdated,pending, andfailedstates.The
AlignedTextcomponent is used appropriately to display the group name alongside a potential financial value derived from thequeryprop.Overall, the component improves the user experience with its visual cues and responsive design. Great job!
packages/desktop-client/src/components/sidebar/Sidebar.tsx (6)
21-31: LGTM!The new icon imports look good and are likely used in the updated sidebar UI.
43-45: LGTM!The new component imports look good and are likely used to organize the sidebar buttons.
149-161: LGTM!The styling changes to the sidebar header look good and are likely made to improve the UI and UX.
173-181: LGTM!Using a separate
ActionButtonscomponent to render the navigation buttons is a good way to keep theSidebarcomponent clean and organized. The button data is passed as a prop, which makes it easy to modify the buttons if needed.
185-189: LGTM!Using a separate
BottomButtonscomponent to render the "Add account" button is a good way to keep theSidebarcomponent clean and organized. The button data is passed as a prop, which makes it easy to modify the buttons if needed.
Line range hint
225-296: LGTM!The changes to the
EditableBudgetNamecomponent look good:
- Removing the "Rename budget" option from the menu and replacing it with a pencil icon is a good way to simplify the UI and make the editing functionality more discoverable.
- The new button uses the
SvgPencil1icon and is styled to be visible only on hover, which is a nice touch.
packages/desktop-client/src/components/sidebar/BottomButtons.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
|
Thanks for putting this up, I've spotted a few minor things:
Notice how the drop down arrow moves to the left and is replaced by the edit name pencil when the mouse is over the budget name? This means a user could be aiming to click the drop down, but when the mouse gets there, it's no longer the drop down menu in that position and this will create a negative experience.
Would also be good to get other opinions. :) |
|
What I like:
What I don't like: As @Teprifer mentions, please re-add the More and Less as indicated, the chevron is kinda just lonely hanging out by itself.
I agree with @Teprifer 's points on de-prioritizing closed accounts with less font-weight and make "all accounts" with more font-weight to give it a little bit more prominence over the accounts below. Great work! An improvement I didn't know I needed but now that I see it, I can't unsee it. |
Updated. Now, if any account group is empty, it will not be shown.
I think we'll disagree on this one. I think keeping More in the middle disrupts the flow of the design. I was trying for an accordion style where clicking the more expands to show more items. If More stays in the middle, why not move those items to a menu for access? Now, with more expanded, all action items are in the same location. More itself is not an action item, so should not be the same size as the rest of the action items. Does anyone else have any thoughts on the More/Less function?
Fixed. Previously, the text in the buttons could be selected. Now, no text on the sidebar can be selected. Everything acts as a button.
Now that I look closely at the chevrons, yes, the alignment is off and I'll work on adjusting the margin/padding. I agree that the account groups should act as accordion components. If everyone agrees with removing the links from For budget/Off budget and making them more like accordion components, I will do that. This change would make it where aligning the chevrons will be easier and I believe this would be a good change for Custom Account Groups too. Personally, I would like the ability to expand/collapse account groups as I have around 15 For budget and another 15 Off budget accounts. Plus I don't look at For budget/Off budget transactions as a group, if I need to I'll use All accounts. Not sure how many people find this {current} feature useful vs losing this feature for expandable/collapsible account groups. Can I get more feedback on whether to remove the links to For budget/Off budget and make the account groups act as accordion components? |
I'm not so sure about outright removing functionality, however, maybe (depending on feedback from others) it could be an option to move it elsewhere. Maybe a hover on for/off budget to have a -> widget to click instead? Dunno, just spit balling. But I think that's stepping outside this PR a bit? Since that'd have to be discussed a bit about how that'd best work. |
|
More/less function
Chevrons @tlesicka and @jfdoming
|
ActionButtons now smoothly expands and collapses.
|
Thank you @Jonathan-Fang
I am glad to know that people use these links.
I tried changing the background when it was clicked/expanded, but didn't really like it. Instead the chevrons now have a hover highlight. @jfdoming, the Action Buttons now have a sliding transition and isn't jumpy. |
|
@tlesicka I wanted to share that the Chevron hover highlight looks great! The action button sliding animation looks great! I'm wondering if we should speed it up to match the snappiness of the rest of the application. Most buttons are click and instant change, so if we made it like the animation 2x-3x as fast, then maybe it would fit better. What does everyone else think about the speed of the animation? |
Changed from 0.4s to 0.15s
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/BottomButtons.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/AccountGroup.tsx
Outdated
Show resolved
Hide resolved
|
👋 Hey! I don't mean to dogpile on this PR, but would you mind splitting up the proposed changes in smaller, more isolated PRs? There are some suggestions that I think are not controversial and would be quickly approved and merged (i.e. the move of "+ add account" to the bottom, budget file edit icon), but others either need more polish, discussion or unfortunately would not be approved entirely. Splitting the proposal up into smaller chunks would allow this to move much more effectively. |
Not a problem. Wasn't sure if I should make smaller but more numerous PRs or one PR that contains more changes.
@MatissJanis what things would not be approved? Asking so that I don't spend time working on something that won't be approved. |
These are the changes I noticed here (I might have missed some): I have ranked them in order according to what I think will receive a lot of good feedback from the community (and thus be merged) to some that would require more fleshing out and/or would likely get a lot of resistance.
|
|
I am late to this party, and I have this very basic question: but what exactly are we trying to solve @tlesicka? The original PR comment does not address this, so please explain this to me. |
@RubenOlsen
As I was working on these things, the scope of the whole PR increased and it was suggested to split this PR into smaller chunks. There are aspects of the side bar that are doubled (settings in two locations), not in line with other aspects of AB (rename budget as menu item instead of pencil icon), or could have a better look/feel when using AB (scrolling and expandable more section). This is what I was trying to solve. This PR is now unused and will be closed soon in favor of smaller PR's with singular scope (such as #3554). |
|
Just a very quick note: @tlesicka thank you for all the care and work you've put into this! It is truly APPRECIATED! As you can see: a lot of us are very opinionated about the side-nav. Which is a good problem to have, but at the same time it can be quite annoying for the person driving the change (I just want to release my change! Stop sending me feedback! AAAhhhhh!!). Please take our feedback comments as positive indicators that people really care about the work you're doing here! |
category: Features
authors: [tlesicka]
Updating Sidebar.
Added collapsible Budget and Off Budget account groups. Removed "Settings" from the "> More" expandible section. Cleaned up "> More" to an up/down chevron. Removed "Rename budget" from menu and added pencil icon next to budget name. Changed scrollable area to just accounts. Action Buttons and All accounts do not scroll.