fix modal scrolling performance on mobile #3343
Conversation
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. |
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
|
tim-smart
changed the title
tim-smart
force-pushed
the
mobile-scroll
branch
2 times, most recently
from
041972c to
e1ee6ef
Compare
|
Can this be more pronounced? Currently its hard to even notice that there is a change at all. |
The goal was to make only a small visual difference, but improve the scrolling speed of the category selector menu on mobile. |
|
Are you using Safari by any chance? I remember implementing something on the PrivacyFilter to fix slow blur performance: |
|
Android / chrome. I tried the forcing gpu rendering trick, but it didn't have any effect. The difference here is that it was using |
tim-smart
force-pushed
the
mobile-scroll
branch
from
1540f8d to
07580e8
Compare
WalkthroughThe pull request introduces modifications to two components: Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (43)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Learnings (1)
🔇 Additional comments (3)
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 (
|
tim-smart
force-pushed
the
mobile-scroll
branch
from
07580e8 to
47b27d4
Compare
![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 (5)
packages/desktop-client/src/components/common/Modal.tsx (1)
Line range hint
238-258: Improved modal button accessibilityThe addition of the
focusButtonprop and its implementation in theModalButtonscomponent is a great accessibility enhancement. It allows for automatic focus on the first visible button when the modal opens, which is particularly useful for keyboard navigation.Consider adding a brief comment explaining the purpose of the
focusButtonprop for better code documentation. For example:// focusButton: When true, automatically focuses the first visible button in the modal focusButton?: boolean;packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (4)
90-91: Approved: Style changes improve scrolling performanceThe changes to the
Viewcomponent's style enhance the scrolling behavior and potentially improve rendering performance:
- Changing
overflow: 'auto'tooverflowY: 'auto'is more specific and allows horizontal content to remain visible if needed.- Adding
willChange: 'transform'can improve performance for transform animations.These modifications align well with the PR objective of enhancing scrolling performance on mobile devices.
Consider wrapping the
willChangeproperty in a condition that checks if the device is mobile, as this optimization is primarily beneficial for mobile devices and might cause unnecessary memory consumption on desktop browsers:style={{ overflowY: 'auto', ...(isMobileDevice && { willChange: 'transform' }), // ... other styles }}This approach ensures that the optimization is applied only when necessary.
Line range hint
265-286: Approved: Enhanced filtering logic improves autocomplete accuracyThe changes to the
filterSuggestionsfunction significantly improve the autocomplete functionality:
- The function now considers both category names and group names when filtering suggestions.
- The use of
getNormalisedStringensures case-insensitive and accent-insensitive comparisons.These enhancements align well with the PR objective of improving the category selector functionality.
Consider extracting the normalization and comparison logic into a separate helper function to improve readability and maintainability:
const normalizedIncludes = (text: string, search: string) => getNormalisedString(text).includes(getNormalisedString(search)); // Then use it in the filter function: return suggestions .filter(suggestion => { if (suggestion.id === 'split') { return true; } if (suggestion.group) { return ( normalizedIncludes(suggestion.group.name, value) || normalizedIncludes(`${suggestion.group.name} ${suggestion.name}`, value) ); } return defaultFilterSuggestion(suggestion, value); }) // ... rest of the functionThis refactoring would make the code more readable and easier to maintain.
Line range hint
420-468: Approved: Enhanced CategoryItem with balance displayThe changes to the
CategoryItemcomponent significantly improve its functionality:
- The component now displays balance information, enhancing the user experience.
- It uses React hooks (
useSyncedPref,useSheetValue) to efficiently manage state and side effects.- The balance display is conditionally rendered based on the
showBalancesprop, providing flexibility.While these changes are not directly related to the PR's main objective of improving scrolling performance, they do enhance the overall functionality of the category selector.
Consider extracting the balance calculation logic into a custom hook to improve code organization and reusability:
function useItemBalance(item: CategoryAutocompleteItem) { const [budgetType = 'rollover'] = useSyncedPref('budgetType'); const balanceBinding = budgetType === 'rollover' ? envelopeBudget.catBalance(item.id) : trackingBudget.catBalance(item.id); return useSheetValue< 'envelope-budget' | 'tracking-budget', typeof balanceBinding >(balanceBinding); } // Then in the CategoryItem component: const balance = useItemBalance(item);This refactoring would make the
CategoryItemcomponent more focused and easier to maintain.
Line range hint
1-468: Overall Assessment: Excellent improvements to CategoryAutocompleteThis PR introduces several valuable enhancements to the
CategoryAutocompletecomponent and its related functions:
- Improved scrolling performance through style optimizations in the
CategoryListcomponent.- Enhanced filtering logic in the
filterSuggestionsfunction, improving autocomplete accuracy.- Added balance display functionality to the
CategoryItemcomponent, enriching the user interface.These changes align well with the PR objectives and contribute to an improved user experience, especially on mobile devices. The code quality is high, with good use of React hooks and TypeScript types.
To further improve the component's architecture and maintainability:
- Consider breaking down the
CategoryAutocomplete.tsxfile into smaller, more focused components. This could improve readability and make the codebase easier to navigate.- Implement proper error handling for the
useSheetValuehook calls to gracefully handle potential data fetching issues.- Add unit tests for the new filtering logic and balance display functionality to ensure long-term reliability.
These architectural improvements would complement the excellent work done in this PR and contribute to the overall robustness of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3343.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (1 hunks)
- packages/desktop-client/src/components/common/Modal.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/src/components/common/Modal.tsx (3)
84-85: Improved modal overlay performanceThe changes to the
ReactAriaModalOverlaycomponent align well with the PR objectives. Replacing thebackdropFilterwith a semi-transparent background color and addingwillChange: 'transform'should significantly improve scrolling performance on mobile devices, especially for the category selector menu.
Line range hint
1-532: Summary of Modal.tsx changesThe changes in this file successfully address the PR objectives:
- Improved scrolling performance by replacing the blur effect with a translucent background.
- Enhanced rendering logic for better performance.
- Improved accessibility with the new
focusButtonprop.These modifications should result in a more efficient and user-friendly modal component, especially on mobile devices. The changes are well-implemented and align with best practices for performance and accessibility.
Line range hint
232-235: Enhanced modal content rendering logicThe addition of conditional styling to hide the modal content when
noAnimationis true andisActiveis false is a good optimization. This should improve performance by not rendering unnecessary content.To ensure this change doesn't introduce any unexpected behavior or accessibility issues, please verify:
- The modal still functions correctly in all scenarios (with and without animation, active and inactive states).
- Screen readers and other assistive technologies can still interact with the modal content appropriately.
You can use the following script to check for potential issues:
✅ Verification successful
Accessibility Concerns Verified
The
Modal.tsxcomponent includes necessaryariaattributes androledeclarations, ensuring proper accessibility support. Additionally, there are no pending accessibility-related TODOs or FIXMEs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential accessibility issues with the new modal rendering logic # Test: Search for aria attributes and roles related to the modal rg --type typescript 'aria-|role=' packages/desktop-client/src/components/common/Modal.tsx # Test: Check if there are any comments or TODOs related to accessibility rg --type typescript '// TODO.*accessibility|// FIXME.*accessibility' packages/desktop-client/src/components/common/Modal.tsxLength of output: 290
Script:
#!/bin/bash # Description: Check for potential accessibility issues with the new modal rendering logic without using unsupported file types # Test: Search for aria attributes and roles related to the modal rg 'aria-|role=' packages/desktop-client/src/components/common/Modal.tsx # Test: Check if there are any comments or TODOs related to accessibility rg '// TODO.*accessibility|// FIXME.*accessibility' packages/desktop-client/src/components/common/Modal.tsxLength of output: 338
tim-smart
force-pushed
the
mobile-scroll
branch
3 times, most recently
from
feebe50 to
c3fe81d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (2)
90-91: Approved: Style changes for improved scrolling performanceThe changes to
overflowY: 'auto'and the addition ofwillChange: 'transform'are good optimizations for improving scrolling performance, especially on mobile devices. This aligns well with the PR's objective.However, be cautious with
willChange: 'transform'as it can increase memory consumption. Consider adding it dynamically only when scrolling starts and removing it when scrolling ends for optimal performance.You might want to consider implementing a scroll listener to dynamically add and remove the
willChangeproperty:const [isScrolling, setIsScrolling] = useState(false); const handleScroll = useCallback(() => { setIsScrolling(true); clearTimeout(scrollTimer.current); scrollTimer.current = setTimeout(() => setIsScrolling(false), 150); }, []); // In the View style willChange: isScrolling ? 'transform' : 'auto', // Add onScroll prop to View onScroll={handleScroll}This approach can further optimize performance by only applying
willChangewhen necessary.
Line range hint
391-394: Approved: Addition of default render functionsThe addition of
defaultRenderCategoryItemGroupHeaderanddefaultRenderCategoryItemfunctions is a great improvement to the component's flexibility. These default implementations allow for easy customization of rendering logic when needed, while providing sensible defaults.Consider adding prop types for the custom render functions in the
CategoryAutocompletePropsinterface to improve type safety:type CategoryAutocompleteProps = { // ... existing props renderCategoryItemGroupHeader?: typeof defaultRenderCategoryItemGroupHeader; renderCategoryItem?: typeof defaultRenderCategoryItem; };This change would ensure that any custom render functions provided must match the signature of the default functions, preventing potential runtime errors.
Also applies to: 449-452
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (52)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--14404-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--321fd-ed-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--4bb70-ed-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--6ab37-roup-name-opens-the-category-group-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--94a79-roup-name-opens-the-category-group-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--94a85-ed-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--9e6aa-in-the-page-header-opens-the-budget-page-menu-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--bbde3-roup-name-opens-the-category-group-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--bed18-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--ceb3a-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--d270d-in-the-page-header-opens-the-budget-page-menu-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--fdd57-in-the-page-header-opens-the-budget-page-menu-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--0ba04-nt-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--1ce6d-nt-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--42062-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--49fb6-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--5f098-roup-name-opens-the-category-group-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--929be-roup-name-opens-the-category-group-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--a3783-in-the-page-header-opens-the-budget-page-menu-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--a8b5e-in-the-page-header-opens-the-budget-page-menu-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--b1562-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--dc927-roup-name-opens-the-category-group-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--f224f-nt-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--f8a19-in-the-page-header-opens-the-budget-page-menu-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3343.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (1 hunks)
- packages/desktop-client/src/components/common/Modal.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/common/Modal.tsx
🧰 Additional context used
🔇 Additional comments (2)
packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (2)
Line range hint
252-270: Approved: Enhanced filtering logic for better autocomplete suggestionsThe updates to the
filterSuggestionsfunction are well-implemented and improve the accuracy of autocomplete suggestions. By considering both category and group names in the filtering process, users will have a better experience finding the categories they need.The implementation is efficient and correctly handles the 'split' suggestion as a special case. The use of
getNormalisedStringensures consistent comparison regardless of string variations.
Line range hint
1-452: Summary: Effective improvements to CategoryAutocomplete componentOverall, the changes made to
CategoryAutocomplete.tsxare well-implemented and align closely with the PR objectives. The key improvements include:
- Enhanced scrolling performance for mobile devices.
- Improved autocomplete suggestion filtering.
- Increased component flexibility with default render functions.
These changes collectively contribute to a better user experience, especially on mobile platforms. The code quality is high, and the implementations follow React best practices.
A few minor suggestions have been made for further optimization and type safety, but these are not critical for the current implementation.
|
Here is some videos showing the difference: telegram-cloud-document-1-5496892831265980427.mp4telegram-cloud-document-1-5496892831265980426.mp4 |
There was a problem hiding this comment.
I can confirm on Mobile Chrome and Edge there's a slight performance improvement.
We lose the blur though and there's not a lot of contrast with some other elements on the desktop screen, particularly in dark mode.
E.g.
What are peoples thoughts on if the style was only applied to mobile?
tim-smart
force-pushed
the
mobile-scroll
branch
from
c3fe81d to
44633fb
Compare
|
I think mobile only is a good option. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (2)
Line range hint
116-117: Improved group header rendering and clarityThe updates to the group header rendering logic are beneficial:
- The new
groupNamevariable clearly indicates hidden groups with a "(hidden)" label.- The simplified
showGroupcondition improves code readability.These changes enhance the user experience by providing more information about hidden groups.
Consider using a template literal for
groupNamefor better readability:const groupName = `${item.group?.name}${item.group?.hidden ? ` ${t('(hidden)')}` : ''}`;
Line range hint
270-290: Enhanced filtering logic for improved autocomplete suggestionsThe updates to the
filterSuggestionsfunction significantly improve the autocomplete functionality:
- The function now considers both category and group names when filtering suggestions.
- The use of
getNormalisedStringensures consistent string comparison, improving matching accuracy.These changes will make it easier for users to find categories, especially when they remember the group name but not the exact category name.
Consider a small optimization to reduce redundant normalization:
const normalizedValue = getNormalisedString(value); return suggestions .filter(suggestion => { if (suggestion.id === 'split') { return true; } const normalizedName = getNormalisedString(suggestion.name); if (suggestion.group) { const normalizedGroupName = getNormalisedString(suggestion.group.name); return normalizedGroupName.includes(normalizedValue) || `${normalizedGroupName} ${normalizedName}`.includes(normalizedValue); } return normalizedName.includes(normalizedValue); }) .sort((a, b) => customSort(a, normalizedValue) - customSort(b, normalizedValue));This optimization reduces the number of times
getNormalisedStringis called, potentially improving performance for large lists of suggestions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (52)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--14404-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--321fd-ed-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--4bb70-ed-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--6ab37-roup-name-opens-the-category-group-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--94a79-roup-name-opens-the-category-group-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--94a85-ed-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--9e6aa-in-the-page-header-opens-the-budget-page-menu-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--bbde3-roup-name-opens-the-category-group-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--bed18-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--ceb3a-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--d270d-in-the-page-header-opens-the-budget-page-menu-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking--fdd57-in-the-page-header-opens-the-budget-page-menu-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Envelope-checks-that-clicking-the-category-name-opens-the-category-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--0ba04-nt-amount-opens-the-budget-summary-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--1ce6d-nt-amount-opens-the-budget-summary-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--42062-in-the-page-header-opens-the-month-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--49fb6-in-the-page-header-opens-the-month-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--5f098-roup-name-opens-the-category-group-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--929be-roup-name-opens-the-category-group-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--a3783-in-the-page-header-opens-the-budget-page-menu-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--a8b5e-in-the-page-header-opens-the-budget-page-menu-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--b1562-in-the-page-header-opens-the-month-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--dc927-roup-name-opens-the-category-group-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--f224f-nt-amount-opens-the-budget-summary-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking--f8a19-in-the-page-header-opens-the-budget-page-menu-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-balance-cell-opens-the-balance-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-budgeted-cell-opens-the-budget-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.mobile.test.js-snapshots/Mobile-Budget-Tracking-checks-that-clicking-the-category-name-opens-the-category-menu-modal-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3343.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (1 hunks)
- packages/desktop-client/src/components/common/Modal.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (2)
90-91: Improved scrolling performance and rendering optimizationThe changes to the
Viewcomponent's style properties are beneficial:
- Changing
overflow: 'auto'tooverflowY: 'auto'allows for more precise control over vertical scrolling while ensuring horizontal content remains fully visible.- Adding
willChange: 'transform'is a performance optimization hint for browsers, potentially improving rendering performance for animations or transitions.These modifications should enhance the scrolling experience and overall performance of the
CategoryListcomponent.
Line range hint
1-524: Overall assessment: Significant improvements to CategoryAutocomplete componentThe changes made to the
CategoryAutocompletecomponent and its related functions bring several notable improvements:
- Enhanced scrolling performance and rendering optimization in the
CategoryListcomponent.- Improved clarity in group header rendering, especially for hidden groups.
- More accurate and user-friendly autocomplete suggestions by considering both category and group names in the filtering logic.
These modifications collectively contribute to a better user experience, improved performance, and more intuitive functionality. The code changes are well-structured and maintain good coding practices.
While some minor optimizations were suggested, the overall quality of the changes is high, and they successfully address the goal of improving the category selector menu's performance and usability.
packages/desktop-client/src/components/common/Modal.tsx (3)
24-24: ImportinguseResponsivefor responsive designThe import of
useResponsivefrom'../../ResponsiveProvider'is correctly added to enable responsive styling based on the viewport size.
55-55: UtilizingisNarrowWidthfor conditional stylingDestructuring
isNarrowWidthfromuseResponsive()allows the component to adjust styles conditionally, enhancing the user experience on different devices.
86-94: Conditional styles improve mobile performanceApplying conditional styles to
ReactAriaModalOverlaybased onisNarrowWidtheffectively replaces thebackdropFilterwith a translucent background color on mobile devices. This addresses performance issues associated with the blur effect on mobile platforms.
There was a problem hiding this comment.
Gotta say I think the blur is doing good work on these screens, without it it makes the menu's look clickable:
Are we sure there's no way to keep the blur but improve the performance? I'm finding it difficult to decide if it's worth losing the blur for the tiny perf improvement.
|
Would |
No it doesn't help here. |
I think it depends on your gpu. I'm using a Pixel 5 and it is quite painful to use the category selector - really choppy and hard to get to the bottom with lots of categories. |
|
I would think that increasing the darkening effect would probably work to be more obvious. You could potentially modify the theme files to have darkening that is theme dependent. |
tim-smart
force-pushed
the
mobile-scroll
branch
from
a6dae7d to
f37c7e4
Compare
This is primarily done by removing the blur effect from the overlay, and replacing it will a translucent black. The blur effect performs poorly on mobile devices.
tim-smart
force-pushed
the
mobile-scroll
branch
from
f37c7e4 to
7a16693
Compare
Will wait for a second review since others have commented.
This is primarily done by removing the blur effect from the overlay, and
replacing it will a translucent black.
The blur effect performs poorly on mobile devices.