Fixes #2885 - Focus ring getting stuck on last column of /accounts/budgeted #3571
Conversation
…creen when creating a new transaction.
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces updates 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: 4
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
2113-2116: Improve readability by formatting function arguments over multiple linesFor better readability and to comply with coding style guidelines, consider formatting the arguments of
useTableNavigatorover multiple lines.Suggested change:
const newNavigator = useTableNavigator( + newTransactions, + getFieldsNewTransaction ); const tableNavigator = useTableNavigator( transactionsWithExpandedSplits, + getFieldsTableTransaction );🧰 Tools
🪛 GitHub Check: lint
[warning] 2113-2113:
ReplacenewTransactions,·getFieldsNewTransactionwith⏎····newTransactions,⏎····getFieldsNewTransaction,⏎··
2181-2181: Remove unnecessary line break before function declarationThere is an unnecessary line break before the function declaration. Placing the
functionkeyword and the function name on the same line improves code consistency.Suggested change:
-function +function getFieldsNewTransaction(item){🧰 Tools
🪛 GitHub Check: lint
[warning] 2181-2181:
Replace⏎··function·getFieldsNewTransaction(item)with··function·getFieldsNewTransaction(item)·
2194-2194: Add a trailing comma after the last element in the arrayAdding a trailing comma after the last element in an array enhances diffs in version control and aligns with style guidelines.
Suggested change:
'add' + ,🧰 Tools
🪛 GitHub Check: lint
[warning] 2194-2194:
Insert,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3571.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/common/Button2.tsx (4 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (5 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/common/Button2.tsx
[warning] 19-19:
../../hooks/useProperFocusimport should occur before import of../../icons/AnimatedLoading
[warning] 20-20:
../../hooks/useMergedRefsimport should occur before import of../../icons/AnimatedLoading
[warning] 147-147:
Replace·children,·variant·=·'normal',·bounce·=·true,·focused·=·false,·...restPropswith⏎······children,⏎······variant·=·'normal',⏎······bounce·=·true,⏎······focused·=·false,⏎······...restProps⏎···
[warning] 151-151:
Delete····packages/desktop-client/src/components/transactions/TransactionsTable.jsx
[warning] 2113-2113:
ReplacenewTransactions,·getFieldsNewTransactionwith⏎····newTransactions,⏎····getFieldsNewTransaction,⏎··
[warning] 2181-2181:
Replace⏎··function·getFieldsNewTransaction(item)with··function·getFieldsNewTransaction(item)·
[warning] 2194-2194:
Insert,
[warning] 2200-2200:
Insert·
🔇 Additional comments (5)
packages/desktop-client/src/components/common/Button2.tsx (4)
4-4: ImportuseRefto manage local refsAdding
useRefis appropriate for managing local refs within the component.
140-140: Addfocusedprop toButtonPropsThe addition of the optional
focusedprop enhances theButtoncomponent's ability to manage focus state based on this prop.
146-152: Implement focus handling correctly with refsThe use of
useRef,useProperFocus, anduseMergedRefscorrectly implements focus management and ref forwarding within theButtoncomponent.🧰 Tools
🪛 GitHub Check: lint
[warning] 147-147:
Replace·children,·variant·=·'normal',·bounce·=·true,·focused·=·false,·...restPropswith⏎······children,⏎······variant·=·'normal',⏎······bounce·=·true,⏎······focused·=·false,⏎······...restProps⏎···
[warning] 151-151:
Delete····
186-186: PassmergedReftoReactAriaButtonfor proper ref handlingPassing
mergedReftoReactAriaButtonensures that both internal and external refs are properly handled.packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
1734-1734: The 'focused' prop is correctly added to 'Cancel' and 'Add' buttonsThe
focusedprop is appropriately set for the 'Cancel' and 'Add' buttons, enabling proper focus handling during tab navigation. This change aligns with the PR objective to fix the focus ring navigation issue.Also applies to: 1754-1754
|
|
||
| const ref = useRef<HTMLButtonElement>(null); | ||
| useProperFocus(ref, focused); | ||
|
|
There was a problem hiding this comment.
Remove unnecessary whitespace
There is unnecessary whitespace on line 151. Please remove it to maintain code cleanliness.
Apply this diff to remove the whitespace:
- 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🧰 Tools
🪛 GitHub Check: lint
[warning] 151-151:
Delete····
| (props, ref) => { | ||
| const { children, variant = 'normal', bounce = true, ...restProps } = props; | ||
| (props, buttonRef) => { | ||
| const { children, variant = 'normal', bounce = true, focused = false, ...restProps } = props; |
There was a problem hiding this comment.
Format destructured props for better readability
For improved readability, format the destructured props over multiple lines as suggested by the linter.
Apply this diff to format the code:
const {
- children, variant = 'normal', bounce = true, focused = false, ...restProps
+ children,
+ variant = 'normal',
+ bounce = true,
+ focused = false,
+ ...restProps
} = props;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { children, variant = 'normal', bounce = true, focused = false, ...restProps } = props; | |
| const { | |
| children, | |
| variant = 'normal', | |
| bounce = true, | |
| focused = false, | |
| ...restProps | |
| } = props; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 147-147:
Replace·children,·variant·=·'normal',·bounce·=·true,·focused·=·false,·...restPropswith⏎······children,⏎······variant·=·'normal',⏎······bounce·=·true,⏎······focused·=·false,⏎······...restProps⏎···
| import { useProperFocus } from '../../hooks/useProperFocus'; | ||
| import { useMergedRefs } from '../../hooks/useMergedRefs'; |
There was a problem hiding this comment.
Reorder imports to satisfy linting rules
The imports of useProperFocus and useMergedRefs should occur before the import of ../../icons/AnimatedLoading to comply with the project's import order conventions.
Apply this diff to fix the import order:
import React, {
forwardRef,
useMemo,
+ useRef,
type ComponentPropsWithoutRef,
type ReactNode,
} from 'react';
import { css } from 'glamor';
+import { useProperFocus } from '../../hooks/useProperFocus';
+import { useMergedRefs } from '../../hooks/useMergedRefs';
import { AnimatedLoading } from '../../icons/AnimatedLoading';
import { type CSSProperties, styles, theme } from '../../style';
import { View } from './View';Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: lint
[warning] 19-19:
../../hooks/useProperFocusimport should occur before import of../../icons/AnimatedLoading
[warning] 20-20:
../../hooks/useMergedRefsimport should occur before import of../../icons/AnimatedLoading
| return getFields(item, fields); | ||
| } | ||
|
|
||
| function getFieldsTableTransaction(item){ |
There was a problem hiding this comment.
Insert a space between 'function' keyword and function name
There should be a space between the function keyword and getFieldsTableTransaction to prevent syntax errors and improve readability.
Suggested change:
-functiongetFieldsTableTransaction(item){
+function getFieldsTableTransaction(item){Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: lint
[warning] 2200-2200:
Insert·
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 No assets were 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 (4)
packages/desktop-client/src/components/common/Button2.tsx (2)
146-157: LGTM: Enhanced focus management in Button componentThe changes to the Button component implementation significantly improve its focus management capabilities:
- The use of
useReffor local ref creation is appropriate.- The
useProperFocushook enhances focus control.- Merging refs with
useMergedRefsallows for both internal management and external access.These changes align with React best practices and improve the component's functionality.
Consider formatting the destructured props for better readability:
const { - children, - variant = 'normal', - bounce = true, - focused = false, - ...restProps + children, + variant = 'normal', + bounce = true, + focused = false, + ...restProps } = props;
Line range hint
1-253: Summary: Improved focus management addresses PR objectivesThe changes made to the Button component successfully address the issue described in the PR objectives. By implementing proper focus management using custom hooks and ref merging, the component now allows for better control of the focus state, which should resolve the problem of the focus ring getting stuck on the last column of the
/accounts/budgetedscreen.Key improvements:
- Added
focusedprop for external focus control.- Implemented
useProperFocushook for internal focus management.- Used
useMergedRefsto combine internal and external refs.These changes enhance the user experience by allowing proper tabbing to the Cancel and Add buttons, as requested in the PR summary.
Regarding the author's request for feedback on potential bad practices:
- The implementation follows React best practices.
- The use of custom hooks and ref management is appropriate and well-executed.
- The changes are minimal and focused on addressing the specific issue.
Consider adding unit tests to verify the new focus management behavior, especially testing the tabbing functionality between different UI elements.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
2113-2121: Improved separation of concerns for transaction fieldsThe separation of field definitions for new transactions and table transactions is a good practice. It allows for more flexibility and potentially different behaviors for each type.
Consider adding a brief comment explaining the purpose of each function for better code documentation:
// Define fields for new transactions const newNavigator = useTableNavigator( newTransactions, getFieldsNewTransaction, ); // Define fields for existing table transactions const tableNavigator = useTableNavigator( transactionsWithExpandedSplits, getFieldsTableTransaction, );
Line range hint
2185-2217: Well-structured field definitions for different transaction typesThe separation of field definitions for new and existing transactions provides flexibility and maintainability. The inclusion of 'cancel' and 'add' fields in
getFieldsNewTransactionaligns well with the UI changes.To reduce code duplication, consider extracting the common fields into a constant:
const commonFields = [ 'select', 'date', 'account', 'payee', 'notes', 'category', 'debit', 'credit', 'cleared' ]; function getFieldsNewTransaction(item) { return getFields(item, [...commonFields, 'cancel', 'add']); } function getFieldsTableTransaction(item) { return getFields(item, commonFields); }This approach makes it easier to maintain the common fields and clearly shows the difference between the two functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/common/Button2.tsx (4 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (5 hunks)
🔇 Additional comments (6)
packages/desktop-client/src/components/common/Button2.tsx (4)
4-4: LGTM: useRef import addedThe addition of
useRefto the React import is appropriate and necessary for the changes made in the Button component implementation.
15-16: LGTM: New hook imports added and correctly orderedThe addition of
useMergedRefsanduseProperFocusimports is appropriate for the enhanced functionality in the Button component. The import order now satisfies the linting rules, resolving the issue mentioned in a previous review comment.
140-140: LGTM: 'focused' prop added to ButtonPropsThe addition of the optional
focusedprop toButtonPropsis appropriate and consistent with the changes in the Button component implementation. This allows for external control of the button's focus state.
191-191: LGTM: Updated ref prop in ReactAriaButtonThe use of
mergedRefas the ref for ReactAriaButton is consistent with the earlier changes and allows the button to work seamlessly with both internal focus management and any external ref passed to the component.packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1734-1737: Improved UI and accessibility for new transaction inputThe addition of 'Cancel' and 'Add' buttons with the 'focused' prop enhances the user interface and improves accessibility by allowing keyboard navigation. This change aligns well with the PR objectives.
Also applies to: 1754-1757
Line range hint
1-2641: Summary of changes and their impactThe modifications in this PR successfully address the issue of focus getting stuck on the last column of the
/accounts/budgetedscreen. The key improvements include:
- Addition of 'Cancel' and 'Add' buttons to the new transaction input UI, enhancing usability and accessibility.
- Separation of field definitions for new and existing transactions, improving code organization and flexibility.
- Implementation of the 'focused' prop for new buttons, enabling proper keyboard navigation.
These changes align well with the PR objectives and enhance the overall user experience. The code structure remains clean and maintainable.
Some minor suggestions for improvement have been made in previous comments, but overall, this is a solid contribution to the codebase.
The-Firexx
changed the title
|
I'm wondering if |
| } = props; | ||
|
|
||
| const ref = useRef<HTMLButtonElement>(null); | ||
| useProperFocus(ref, focused); |
There was a problem hiding this comment.
We should use this hook on the TransactionsTable and just pass the ref to the Add and Cancel button instead of doing it directly in the Button component.
There was a problem hiding this comment.
Changed the source code to avoid messing with Button2, can you check? Thank you
| @@ -1670,6 +1671,11 @@ function NewTransaction({ | |||
| ); | |||
| const emptyChildTransactions = childTransactions.filter(t => t.amount === 0); | |||
|
|
|||
| const addRef = useRef(null); | |||
There was a problem hiding this comment.
| const addRef = useRef(null); | |
| const addButtonRef = useRef(null); |
| @@ -1670,6 +1671,11 @@ function NewTransaction({ | |||
| ); | |||
| const emptyChildTransactions = childTransactions.filter(t => t.amount === 0); | |||
|
|
|||
| const addRef = useRef(null); | |||
| useProperFocus(addRef, focusedField === 'add'); | |||
| const cancelRef = useRef(null); | |||
There was a problem hiding this comment.
| const cancelRef = useRef(null); | |
| const cancelButtonRef = useRef(null); |
| @@ -1731,6 +1737,7 @@ function NewTransaction({ | |||
| style={{ marginRight: 10, padding: '4px 10px' }} | |||
| onPress={() => onClose()} | |||
| data-testid="cancel-button" | |||
| ref={cancelRef} | |||
There was a problem hiding this comment.
| ref={cancelRef} | |
| ref={cancelButtonRef} |
| @@ -1750,6 +1757,7 @@ function NewTransaction({ | |||
| style={{ padding: '4px 10px' }} | |||
| onPress={onAdd} | |||
| data-testid="add-button" | |||
| ref={addRef} | |||
There was a problem hiding this comment.
| ref={addRef} | |
| ref={addButtonRef} |
|
Sorry for the very long delay. |
This pull request is to allow a user to use TAB to reach the Cancel and Add button, which cause a better user experience.
This fixes #2885
The changes I made are minimal, however if it is necessary to create/edit tests or other things please let me know.
Also, I'm not very used to React, so any "bad practice" please also report.
Thank you.