Update Sidebar - Refactor the Budget Name component #3593
Conversation
Moved Budget Name to its own component for a cleaner Sidebar component. Added pencil icon for editing budget name. Removed Rename Budget from menu.
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. |
WalkthroughThe pull request introduces a new React component, Additionally, the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
126-131: LGTM: Hover styles added for improved user interaction.The new CSS styles for
.hover-visibleelements align with the PR objectives and provide a clean way to show/hide elements on hover. This is likely used for the new pencil icon next to the budget name.Consider adding a
transitionproperty to smooth out the appearance/disappearance of the hover-visible elements:'& .hover-visible': { display: none, opacity: 0, transition: opacity 0.2s ease-in-out, }, '&:hover .hover-visible': { display: flex, opacity: 1, }This will create a fade-in effect, potentially improving the user experience.
136-140: LGTM: BudgetName component integration looks good.The replacement of
EditableBudgetNamewith the newBudgetNamecomponent aligns with the PR objectives and improves code organization. TheToggleButtonfunctionality is preserved within the new structure.Consider extracting the
ToggleButtonrendering logic into a separate variable for improved readability:const toggleButton = !sidebar.alwaysFloats && ( <ToggleButton isFloating={isFloating} onFloat={onFloat} /> ); return ( ... <BudgetName> {toggleButton} </BudgetName> ... );This separation of concerns can make the JSX structure cleaner and easier to understand at a glance.
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
130-132: Handle long budget names gracefully.The budget name is displayed within a
Textcomponent withwhiteSpace: 'nowrap'andoverflow: 'hidden', which will truncate long names without indication. Consider adding text-overflow ellipsis to indicate that the name is truncated.Apply this style change to indicate truncated text:
<Text style={{ whiteSpace: 'nowrap', overflow: 'hidden' }}> + style={{ + whiteSpace: 'nowrap', + overflow: 'hidden', + textOverflow: 'ellipsis', + }}
102-109: Provide feedback when the budget name is empty.Currently, if the user enters an empty budget name, the input is ignored. Consider providing visual feedback or a validation message to inform the user.
Implement a simple alert or message:
if (newBudgetName.trim() !== '') { setBudgetNamePref(newBudgetName); setEditing(false); + } else { + alert(t('Budget name cannot be empty')); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3593.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/sidebar/BudgetName.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
1-1: LGTM: Import statements have been cleaned up and updated.The removal of unused imports and the addition of the
BudgetNameimport align with the PR objectives and improve code cleanliness.Also applies to: 7-7, 21-21
Line range hint
1-168: Overall implementation aligns well with PR objectives.The changes in this file successfully achieve the following PR objectives:
- Removal of the "Rename budget" option from the sidebar menu.
- Introduction of the
BudgetNamecomponent, which likely contains the new pencil icon functionality.- Refactoring of the budget name into its own component, improving code organization.
The sidebar structure has been updated appropriately to accommodate these changes, and the overall implementation appears to follow the requested design changes.
To ensure that the
BudgetNamecomponent is implemented correctly, please run the following command to check its contents:This will help confirm that the pencil icon and dynamic resizing functionality are properly implemented in the new component.
✅ Verification successful
Verified the
BudgetNamecomponent correctly implements the pencil icon and dynamic resizing functionality as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the BudgetName component # Test: Check the contents of the BudgetName component cat packages/desktop-client/src/components/sidebar/BudgetName.tsxLength of output: 4352
packages/desktop-client/src/components/sidebar/BudgetName.tsx (3)
89-89: Confirm that 'Close file' menu item works correctly in all environments.The 'Close file' option might behave differently depending on the platform. Verify that it functions as expected in both browser and desktop environments.
Run the following script to check usage of
closeBudgetaction:#!/bin/bash # Description: Verify the implementation of 'closeBudget' action. # Search for the 'closeBudget' action definition and its handling. rg --type typescript 'export function closeBudget' -A 10 # Check where 'closeBudget' is handled in reducers or sagas. rg --type typescript 'case.*CLOSE_BUDGET' -A 5
145-158: Improve accessibility by addingaria-labelto the edit button.While an
aria-labelis already provided, ensure that theButtoncomponent passes this prop to the underlying element for screen readers.Run the following script to confirm that
aria-labelis properly handled:#!/bin/bash # Description: Verify that 'Button' passes 'aria-label' to the DOM. # Search for 'aria-label' in the 'Button' component implementation. rg --type typescript 'export const Button' -A 20 | rg 'aria-label'
119-134: Verify thattriggerRefis correctly assigned to the Button component.Ensure that the
refprop is properly passed to the underlying DOM element. If theButtoncomponent does not forward refs to the native button element,triggerRefmay not function as intended, affecting the positioning of thePopover.Run the following script to check if
Buttonforwards refs:
tlesicka
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
60-90: Good use of hooks and conditional rendering!The
EditableBudgetNamecomponent makes good use of various hooks for state management and functionality. The conditional rendering of menu items based on the platform is a nice touch for cross-platform compatibility.A minor suggestion for improvement:
Consider using an object lookup instead of a switch statement in the
onMenuSelectfunction for better scalability:const menuActions = { settings: () => navigate('/settings'), help: () => window.open('https://actualbudget.org/docs/', '_blank'), close: () => dispatch(closeBudget()), }; function onMenuSelect(type: string) { setMenuOpen(false); menuActions[type]?.(); }This approach makes it easier to add new menu items in the future without modifying the function body.
92-114: Good implementation of editable budget name!The conditional rendering based on the editing state and the use of
InitialFocusfor automatically focusing the input field enhance the user experience. The input handlers appropriately update the budget name and exit editing mode.A minor suggestion for improvement:
Consider adding a maximum length validation to prevent excessively long budget names:
onEnter={e => { const inputEl = e.target as HTMLInputElement; const newBudgetName = inputEl.value.trim(); const maxLength = 50; // Or any other appropriate maximum length if (newBudgetName !== '' && newBudgetName.length <= maxLength) { setBudgetNamePref(newBudgetName); setEditing(false); } else { // Optionally, show an error message to the user } }}This ensures that budget names remain within a reasonable length.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/sidebar/BudgetName.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (3)
Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:64-64 Timestamp: 2024-10-07T11:45:10.127Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, when `useMetadataPref('budgetName')` returns `undefined`, the code handles it appropriately without the need to assign a default value to `budgetName`.Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:88-88 Timestamp: 2024-10-07T11:46:26.033Z Learning: In the codebase, arrays do not handle `false` or `undefined` values appropriately. Therefore, when conditionally including items in arrays, we should avoid including `false` or `undefined` elements.Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:74-78 Timestamp: 2024-10-07T11:30:25.900Z Learning: In `BudgetName.tsx`, the help documentation must open in a new window, so using `window.open('https://actualbudget.org/docs/', '_blank')` is acceptable.
🔇 Additional comments (3)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (3)
1-23: Imports and type definition look good!The imports cover all necessary dependencies for the component's functionality, including React hooks, Redux, custom hooks, icons, and UI components. The
BudgetNamePropstype definition is simple and appropriate, allowing for optional children.
25-58: Component structure looks good!The
BudgetNamecomponent is well-structured, determining the layout based on the platform and allowing for flexible content with optional children. The use ofEditableBudgetNameas a child component promotes good separation of concerns.
1-165: Overall, excellent implementation of the BudgetName component!The
BudgetNameandEditableBudgetNamecomponents are well-structured and follow good React practices. They make appropriate use of hooks for state management and side effects, and consider cross-platform compatibility in their implementation.Key strengths:
- Clear separation of concerns between the main
BudgetNamecomponent and the editable part.- Good use of conditional rendering and platform-specific adjustments.
- Thoughtful UX considerations, such as auto-focusing the input field when editing.
Areas for potential improvement:
- Moving complex inline styles to a separate CSS file or using a CSS-in-JS solution for better maintainability.
- Enhancing accessibility for touch devices where hover states are not applicable.
- Adding input validation for the budget name to prevent excessively long names.
These minor improvements aside, the overall quality of the implementation is high, and it should serve its purpose well within the application.
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
115-119: Approve BudgetName component usage with a minor suggestion.The replacement of the
EditableBudgetNamefunction with theBudgetNamecomponent is a good refactoring decision. It simplifies the Sidebar component and likely improves the separation of concerns.Consider adding a comment explaining the relationship between
BudgetNameandToggleButtonto improve code readability:<BudgetName> {/* ToggleButton is placed here for layout purposes, allowing it to be positioned relative to the budget name */} {!sidebar.alwaysFloats && ( <ToggleButton isFloating={isFloating} onFloat={onFloat} /> )} </BudgetName>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (2 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:64-64 Timestamp: 2024-10-07T11:45:10.127Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, when `useMetadataPref('budgetName')` returns `undefined`, the code handles it appropriately without the need to assign a default value to `budgetName`.Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:64-64 Timestamp: 2024-10-08T15:46:15.739Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, when `useMetadataPref('budgetName')` returns `undefined`, the code handles it appropriately without the need to assign a default value to `budgetName`.
🪛 GitHub Check: lint
packages/desktop-client/src/components/sidebar/Sidebar.tsx
[warning] 13-13:
'SvgReports' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'SvgWallet' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
[warning] 14-14:
'SvgCalendar' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 15-15:
../../icons/v0import should occur before import of../../icons/v1
[warning] 15-15:
'SvgExpandArrow' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 16-16:
../../icons/v1import should occur before import of../../icons/v2
[warning] 16-16:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
🔇 Additional comments (4)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (4)
105-110: Approve CSS style changes for hover visibility.The addition of hover visibility styles is a good improvement to the user interface. It allows for a cleaner default view while still providing access to additional functionality when needed.
The use of nested selectors (e.g.,
'&:hover .hover-visible') is a clean way to implement this behavior. Good job on maintaining consistency with the existing code style and using theme variables.
Line range hint
1-143: Overall assessment: Good refactoring with minor improvements needed.The changes to the Sidebar component represent a positive refactoring effort:
- The introduction of the
BudgetNamecomponent improves code organization and separation of concerns.- New hover visibility styles enhance the user interface, providing a cleaner default view with accessible additional functionality.
- The overall structure of the Sidebar component has been simplified.
To further improve the code:
- Address the unused imports and reorganize import statements for better clarity.
- Consider adding comments to explain the relationship between components, especially for non-obvious nesting (e.g., ToggleButton inside BudgetName).
These changes contribute to a more maintainable and user-friendly sidebar component. Good job on the refactoring effort!
🧰 Tools
🪛 GitHub Check: lint
[warning] 13-13:
'SvgReports' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'SvgWallet' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
[warning] 14-14:
'SvgCalendar' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 15-15:
../../icons/v0import should occur before import of../../icons/v1
[warning] 15-15:
'SvgExpandArrow' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 16-16:
../../icons/v1import should occur before import of../../icons/v2
[warning] 16-16:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
1-25:⚠️ Potential issueAddress linter warnings.
The static analysis tool has flagged several issues that should be addressed:
- Unused imports: SvgReports, SvgWallet, SvgCalendar, and SvgExpandArrow are imported but not used in this file.
- Import order: The order of imports could be improved for better organization.
To resolve these issues:
Remove the unused imports if they're not needed in this file:
// Remove these lines if the imports are not used import { SvgReports, SvgWallet } from '../../icons/v1'; import { SvgCalendar } from '../../icons/v2'; import { SvgExpandArrow } from '../../icons/v0';Reorganize the imports as suggested in the previous comment about import organization.
To confirm that removing these imports doesn't affect the component, run:
#!/bin/bash # Check for usage of SvgReports, SvgWallet, SvgCalendar, and SvgExpandArrow rg --type typescript "SvgReports|SvgWallet|SvgCalendar|SvgExpandArrow" packages/desktop-client/src/components/sidebar/Sidebar.tsx # Check for any SVG usage in the file rg --type typescript "Svg[A-Z]" packages/desktop-client/src/components/sidebar/Sidebar.tsxThis will help ensure that removing the imports won't break any functionality.
🧰 Tools
🪛 GitHub Check: lint
[warning] 13-13:
'SvgReports' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'SvgWallet' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
[warning] 14-14:
'SvgCalendar' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 15-15:
../../icons/v0import should occur before import of../../icons/v1
[warning] 15-15:
'SvgExpandArrow' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 16-16:
../../icons/v1import should occur before import of../../icons/v2
[warning] 16-16:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
1-25: Improve import organization and remove unused imports.The import statements have been updated, but there are a few improvements we can make:
- The import order could be more consistent. Consider grouping imports by external libraries, then internal modules, and finally local components.
- Some newly added imports (SvgReports, SvgWallet, SvgCalendar) are flagged as unused by the linter. If they're not used in this file, consider removing them.
Here's a suggested reorganization of the imports:
import React, { useState } from 'react'; import { useTranslation } from 'react-i18next'; import { useDispatch } from 'react-redux'; import { Resizable } from 're-resizable'; import { replaceModal } from 'loot-core/src/client/actions'; import * as Platform from 'loot-core/src/client/platform'; import { useGlobalPref } from '../../hooks/useGlobalPref'; import { useLocalPref } from '../../hooks/useLocalPref'; import { useResizeObserver } from '../../hooks/useResizeObserver'; import { useResponsive } from '../../ResponsiveProvider'; import { styles, theme } from '../../style'; import { SvgAdd } from '../../icons/v1'; import { View } from '../common/View'; import { Accounts } from './Accounts'; import { BudgetName } from './BudgetName'; import { PrimaryButtons } from './PrimaryButtons'; import { SecondaryButtons } from './SecondaryButtons'; import { useSidebar } from './SidebarProvider'; import { ToggleButton } from './ToggleButton'; // Remove the following if they're not used in this file: // import { SvgReports, SvgWallet } from '../../icons/v1'; // import { SvgCalendar } from '../../icons/v2'; // import { SvgExpandArrow } from '../../icons/v0';To confirm the usage of potentially unused imports, run:
✅ Verification successful
Remove unused SVG imports from Sidebar.tsx.
The imports for
SvgReports,SvgWallet,SvgCalendar, andSvgExpandArroware not used within theSidebar.tsxcomponent. Removing them will clean up the code and eliminate unnecessary dependencies.// Remove the following imports if they are not used in this file: import { SvgReports, SvgWallet } from '../../icons/v1'; import { SvgCalendar } from '../../icons/v2'; import { SvgExpandArrow } from '../../icons/v0';🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of SvgReports, SvgWallet, SvgCalendar, and SvgExpandArrow rg --type typescript "SvgReports|SvgWallet|SvgCalendar|SvgExpandArrow" packages/desktop-client/src/components/sidebar/Sidebar.tsxLength of output: 294
Script:
#!/bin/bash # Check for usage of SvgReports, SvgWallet, SvgCalendar, and SvgExpandArrow in Sidebar.tsx rg "SvgReports|SvgWallet|SvgCalendar|SvgExpandArrow" packages/desktop-client/src/components/sidebar/Sidebar.tsxLength of output: 264
🧰 Tools
🪛 GitHub Check: lint
[warning] 13-13:
'SvgReports' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'SvgWallet' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 13-13:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
[warning] 14-14:
'SvgCalendar' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 15-15:
../../icons/v0import should occur before import of../../icons/v1
[warning] 15-15:
'SvgExpandArrow' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 16-16:
../../icons/v1import should occur before import of../../icons/v2
[warning] 16-16:
'/home/runner/work/actual/actual/packages/desktop-client/src/icons/v1/index.ts' imported multiple times
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
102-107: LGTM: New hover styles improve UI interaction.The addition of
.hover-visiblestyles enhances the user interface by showing certain elements only on hover, which can help reduce visual clutter. This is a good UI practice.Consider using CSS transitions for a smoother appearance/disappearance effect:
'& .hover-visible': { opacity: 0, transition: opacity 0.2s ease-in-out, }, '&:hover .hover-visible': { opacity: 1, }This would provide a more polished user experience.
Line range hint
1-141: Overall changes improve Sidebar component structure and maintainability.The modifications to the Sidebar component focus on improving its structure and organization without significantly altering its core functionality. The introduction of the
BudgetNamecomponent and the removal ofEditableBudgetNamealign well with the PR objectives of refactoring the budget name into its own component.Key improvements:
- Better code organization with the new
BudgetNamecomponent.- Enhanced UI with new hover visibility styles.
- Cleaned up import statements.
These changes should contribute to better maintainability and potentially improved performance due to more focused component responsibilities.
Consider further decomposing the Sidebar component if it grows in complexity. Each logical section (e.g., Accounts, PrimaryButtons, SecondaryButtons) could potentially be its own component, improving modularity and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (2 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:64-64 Timestamp: 2024-10-07T11:45:10.127Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, when `useMetadataPref('budgetName')` returns `undefined`, the code handles it appropriately without the need to assign a default value to `budgetName`.Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:64-64 Timestamp: 2024-10-08T15:46:15.739Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, when `useMetadataPref('budgetName')` returns `undefined`, the code handles it appropriately without the need to assign a default value to `budgetName`.
🔇 Additional comments (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
1-1: LGTM: Import statements have been cleaned up and updated.The changes to the import statements improve code cleanliness by removing unused imports and adding the necessary
BudgetNamecomponent. This aligns well with the modifications made to theSidebarcomponent structure.Also applies to: 7-7, 19-19
|
Thank you and LGTM! |
|
This is an interesting idea. I might be a bit weird, but I'm finding the placement a little strange. I think the pencil icon works when it's next to a text field because it's clear that you'd edit the text, but I've never seen a pencil icon next to a dropdown menu before. I'm not totally against it, I just think it looks odd and I don't see what's wrong with the current implementation. I'm not known for my UI prowess though, I'm wondering what others think 😅. |
I did get a similar vibe but wasn't gonna comment until I saw yours :) PR at time of this comment: MS Paint Mock of suggestion: |
|
@MikesGlitch and @Teprifer thanks for the comments. My initial thought for the rename input box location was the same as yours for the pencil icon.
I haven't seen a dropdown with an input box. I've seen either a popup dialog or an option in the settings. Both of which are beyond the scope of what I was initially trying to accomplish. I'm ok with reverting back to "Rename budget" as a menu item in the dropdown. I'll leave this PR as there's a structural code change that I believe is important (separating @Jonathan-Fang, @jfdoming, @MatissJanis any thought? |
|
Ok, since this is just a refactor now, could you checkout the existing VRT images from master to clean up the change list? That way we can make sure things really aren't changing. Ill look over the code changes too. |
|
I looked over it since its pretty straight forward and it seems good |
youngcw
changed the title
Continuation of splitting #3457 into several PR's as requested by @MatissJanis.
This PR removes "Rename budget" from the menu and replaces it with a pencil icon next to the budget name. This updates the sidebar to the same look/feel as renaming an account from the account page. The rename input box will now resize with the sidebar width instead of a fixed 160px.
Budget name is now it's own component which cleans up the Sidebar code.