fix: render empty report preview placeholders without waiting for width#86272
fix: render empty report preview placeholders without waiting for width#86272emkhalid wants to merge 3 commits intoExpensify:mainfrom
Conversation
|
@ZhenjaHorbach 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] |
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a1137688
ℹ️ 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".
| const showStatusAndSkeleton = !shouldShowEmptyPlaceholder; | ||
| // Empty/access placeholders do not depend on measured carousel width, so we can show them immediately | ||
| // instead of keeping the taller loading state around and causing the preview to reflow. | ||
| const shouldShowPreviewLoading = shouldShowLoading || (shouldShowLoadingDeferred && !shouldShowPreviewPlaceholder) || (!currentWidth && !shouldShowPreviewPlaceholder); |
There was a problem hiding this comment.
Preserve deferred loading before showing empty placeholder
This change bypasses shouldShowLoadingDeferred whenever shouldShowEmptyPlaceholder is true, but the component explicitly documents that hasOnceLoadedReportActions can flip to true before transactions are populated. In that transition (hasOnceLoadedReportActions=true, transactions=[], deferred still true), the preview now renders the empty placeholder immediately and can briefly show an incorrect empty state before real transactions appear, causing a visual flicker/regression on non-empty reports.
Useful? React with 👍 / 👎.
|
@ZhenjaHorbach I’ve finished the implementation, completed the required checks I could perform, and updated the PR description/checklist accordingly. This is my first PR in this repo, so if there is anything else you’d like me to update or any repo-specific step I’m still missing, please let me know and I’ll address it right away. I’d appreciate your review whenever you have time. thanks |
@emkhalid And thanks for the PR |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Makes sense!
Explanation of Change
This PR $ #85769 by removing the unnecessary width gate for placeholder-only preview states in the money request report preview.
Previously, empty/access placeholder states were still forced through the loading branch while
currentWidthwas0, so the preview briefly rendered the taller loading container before shrinking to the placeholder. That height change caused the visible jump in workspace chat.This change keeps the loading state only for the real carousel-loading case and renders the empty/access placeholders immediately when those states are already known.
Fixed Issues
$ #85769
PROPOSAL: #85769 (comment)
Tests
Offline tests
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
no-preview-jump-android-netive.mov
Android: mWeb Chrome
no-preview-jump-android-web.mov
iOS: Native
no-preview-jump-ios-native.mov
iOS: mWeb Safari
no-preview-jump-ios-web.mov
MacOS: Chrome / Safari
no-preview-jump-mac-os.mov