fix: show map receipt full hight in Wide RHP #78980
Conversation
melvin-bot
bot
requested review from
JmillsExpensify and
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 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] |
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.
|
|
@abdulrahuman5196, this pr is ready for your review. please take a look when you get a chance. thanks |
|
@abdulrahuman5196, kindly bump. thanks |
| }} | ||
| source={typeof source === 'string' ? {uri: source} : source} | ||
| style={[style ?? [styles.w100, styles.h100], styles.overflowHidden]} | ||
| style={isMapDistanceRequest && styles.flex1} |
There was a problem hiding this comment.
why are we changing existing styles in other flow if we are having isMapDistanceRequest logic? @nabi-ebrahimi Can we not do that to avoid further regressions?
There was a problem hiding this comment.
@abdulrahuman5196, those styles were never applied to the component, so I’ve removed them.
There was a problem hiding this comment.
I don't think we should remove any existing logics, at this point even if small. Since the PRs raised have already caused couple of regression.
I would highly recomend to only change what is absolutely required.
@nabi-ebrahimi
There was a problem hiding this comment.
@abdulrahuman5196, thanks for the feedback — I understand the concern about minimizing changes, especially given recent regressions. I’d like to clarify why removing the parent styles is actually the safer option here.
Before
The parent was passing styles that were effectively not applied by the child:
// Parent
style={[style ?? [styles.w100, styles.h100], styles.overflowHidden]}// Child
{...rest}
style={[styles.w100, styles.h100]}Because the child component did not consume the style prop, any styles coming from the parent (including overflowHidden) were ignored.
After
I updated the child to properly apply the style prop:
// Child
{...rest}
style={[styles.w100, styles.h100, style]}Once this change is made, all parent styles start applying for the first time. If we keep the old parent styles unchanged, they will suddenly take effect and may introduce regressions.
To avoid that, I simplified the parent to only pass what is explicitly needed:
// Parent
style={isMapDistanceRequest && styles.flex1}So while this does remove existing logic in the parent, that logic was previously unused. Keeping it would actually be riskier now, because it would introduce new behavior rather than preserve existing behavior.
For this reason, I believe removing those parent styles is the safer and more minimal change in practice. Happy to revisit if you see an alternative approach that avoids introducing new styling behavior. Thanks!
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Product polish looks great!
|
@abdulrahuman5196, quick bump. thanks |
1 similar comment
|
@abdulrahuman5196, quick bump. thanks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-29.at.12.35.38.AM.movAndroid: mWeb ChromeScreen.Recording.2026-01-29.at.12.40.35.AM.moviOS: HybridAppScreen.Recording.2026-01-29.at.12.20.15.AM.moviOS: mWeb SafariScreen.Recording.2026-01-29.at.12.30.16.AM.movMacOS: Chrome / SafariScreen.Recording.2026-01-28.at.11.49.02.PM.mov |
abdulrahuman5196
left a comment
abdulrahuman5196
left a comment
There was a problem hiding this comment.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @inimaga
🎀 👀 🎀
C+ Reviewed
There was a problem hiding this comment.
Adjacent to the raised comment/conversation earlier. I think we should get rid of "style" as it is effectively dead code. The type definition above, this prop, and its corresponding usage later.
Otherwise looks good. And ready to go.
There was a problem hiding this comment.
@inimaga, thanks for the review. applied.
|
@inimaga, pr is ready for your review. please take another look thanks. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.3.11-16 🚀
|
5 participants
Explanation of Change
These changes will ensure that the map distance request receipt is displayed at full width and height in the wide RHP.
Fixed Issues
$ #72191
PROPOSAL: #72191 (Comment)
Tests
→ The distance receipt preview fills the area completely while the map is updating.
Offline tests
Same as 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 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
Screen.Recording.1404-09-08.at.1.46.43.PM.mov
Android: mWeb Chrome
Screen.Recording.1404-09-08.at.1.49.34.PM.mov
iOS: Native
ios-native.mp4
iOS: mWeb Safari
Screen.Recording.1404-09-08.at.1.54.09.PM.mp4
MacOS: Chrome / Safari
Screen.Recording.1404-10-17.at.12.22.30.PM.mp4