[CP Staging] Fix selection, keyboard nav, and highlight regressions in group-by split rendering#93540
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Sorry for the bump @DylanDylann, @ZhenjaHorbach will review this. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 457b83fa21
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… on web Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…imation Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
JS00001
left a comment
There was a problem hiding this comment.
Lets go ahead w/ merge here, maybe we can follow up to clean code, but not too important
| for (let i = 0; i < splitData.length; i++) { | ||
| const item = splitData.at(i); | ||
| if (item && isGroupChildrenContainerItem(item)) { | ||
| childrenIndices.push(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
NAB, prob simpler to do:
| for (let i = 0; i < splitData.length; i++) { | |
| const item = splitData.at(i); | |
| if (item && isGroupChildrenContainerItem(item)) { | |
| childrenIndices.push(i); | |
| } | |
| } | |
| for (const [item, index] of splitData.entries()) { | |
| if (isGroupChildrenContainerItem(item)) { | |
| childrenIndices.push(index); | |
| } | |
| } |
…remains [CP Staging] Fix selection, keyboard nav, and highlight regressions in group-by split rendering (cherry picked from commit 04ddc38) (cherry-picked to staging by puneetlath)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.4.7-3 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR and they are all internal regression fixes to the group-by split rendering in the Search list — none of them alter customer-facing features, flows, settings, labels, or copy that the help site documents. Why no docs update is neededThe diff is limited to behavioral/visual bug fixes in the
No new strings, labels, buttons, or user-facing copy were added (the only string-like changes are code symbols and theme color references). Help site articles under Since no documentation changes are required, I did not create a draft PR. @Krishna2323, please confirm you agree no help site updates are needed here. If you believe a specific article should be updated, let me know which behavior is customer-facing and I'll draft the changes. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.7-4 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — no changes requiredI reviewed the changes in this PR against the help site files under Conclusion: No help site updates are required. Why: This PR is a regression fix for the group-by split-rendering work from #92067. All changes are internal rendering/interaction fixes in
These restore previously-intended behavior. They do not introduce any new feature, setting, tab, button, or label, and they do not change any documented product behavior. Help articles describe how to use features (e.g., searching, grouping, and selecting expenses), and that documented behavior is unchanged by these fixes — so there is nothing to add or correct in the docs, and no draft PR was created. @Krishna2323, since no help site changes are required, there's no linked help site PR to review. If you believe a specific article should change, let me know which behavior is documented incorrectly and I'll create the draft PR. |
|
🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no changes required. I reviewed the changes in this PR against the help site articles under Why: This is a bug-fix-only PR. All 8 changed files are internal frontend code —
None of these introduce, rename, or alter a user-facing feature, setting, tab, or workflow. The @Krishna2323, if you believe a specific article should be updated to reflect behavior changed here, let me know which one and I'll draft the changes. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.8-3 🚀
|
Explanation of Change
#93526 — Only expanded headers were sticky, so the last one stayed stuck with nothing to push it off. Fix: make all group headers sticky so collapsed ones push the previous header away.
#93527 —
SearchListstopped counting lazy-loadedgroupBygroups as selectable, andGroupHeadertreated unloaded groups as empty. Fix: restore pre-#92067 selection counting and don’t disable groups that havetransactionsQueryJSON.#93534 — Split rendering added invisible
children_containerrows between headers, so arrow keys needed two presses. Fix: skip those indices in keyboard nav and clean up border styling so the focus box renders fully.#93535 — Selection highlight/border only applied to the header after the split. Fix: align selection styling so the full group (header + children) highlights together.
#93536 — New-expense highlight lived on the old single wrapper; after the split only
GroupHeaderanimated. Fix: adduseAnimatedHighlightStyletoGroupChildrenContainerso the expanded section highlights too.Fixed Issues
$ #88926
$ #93526
$ #93527
$ #93534
$ #93535
$ #93536
PROPOSAL:
Tests
Test #93526
Test #93527
Test #93534
-Account has many expenses from different users.
Test for #93535
Test for #93536
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
new_expense_highlight_issue.mp4
remaining_bugs.mp4