-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update padding used in map #47548
Update padding used in map #47548
Conversation
@marcaaron Is it possible to add "Ready to Build" label so that we can get adhoc builds? |
Can you build it locally or no? |
Unable to build it as of now.
|
@marcaaron Bump on the above. |
What issue are you having exactly? Can you provide more details or seek help in Slack? I'll do it once for now - but please understand it's not really something we can do every time. |
Please also sign your commits. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@ikevin127 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] |
@marcaaron I was developing on a Windows system temporarily, and was facing some build issues. @ikevin127 Added screenshots, will these be enough or do we need on all platforms? |
@ShridharGoel I guess, I'll move on with the Reviewer Checklist as is. Just make sure to sign your commits because we won't be able to merge because of the Important You're missing testing steps from the PR description❗ |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.webmAndroid: mWeb Chromeandroid-mweb.webmiOS: Nativeios.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
src/CONST.ts
Outdated
@@ -2588,7 +2588,7 @@ const CONST = { | |||
PINK: 'Pink', | |||
}, | |||
|
|||
MAP_PADDING: 50, | |||
MAP_PADDING: 32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAP_PADDING: 32, |
Let's remove the duplicated MAP_PADDING
variable and only keep MAPBOX.PADDING
which we should use in both places where <DistanceMapView />
is used, since both variables are only used once, for the same purpose.
This comment was marked as outdated.
This comment was marked as outdated.
@ShridharGoel I completed the PR Reviewer Checklist, tests well 👍 Once you address #47548 (comment) and #47548 (comment) I will be able to 🟢 Approve. |
7a78f63
to
a07cce4
Compare
fafb51e
to
a07cce4
Compare
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Thanks for addressing the comments from #47548 (comment), PR tests well and looks good for merge!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
@marcaaron The above-reported performance issue was caused by another PR (#46645) for which I just reviewed the fix PR (#47891) today which is pending performance testing and merge. The same report was posted on all recently merged PRs which are in sync with main. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.25-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.25-0 🚀
|
Details
Update padding used in map.
Fixed Issues
$ #46296
PROPOSAL: #46296 (comment)
Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop