-
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
Use Image instead of ImageBackground #15391
Conversation
@aimane-chnaif @MariaHCD One of you needs to 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] |
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.
These changes make sense to me! Tagging @shawnborton to double check the style changes.
This looks good to me, also cc @dangrous who worked on this originally. |
Ha! My original solution strikes again... So weird that ImageBackground doesn't behave like a background should, but if the Image component still looks the same, I'm totally fine with that. Seems like it does from the screenshots. |
@MariaHCD @aimane-chnaif could one of you complete the Reviewer Checklist, please? |
Sure, I will review and complete checklist during weekend. |
Reviewer Checklist
Screenshots/Videos |
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.
Current solution looks good to me.
There are also alternative options to fix this issue:
-
Option 1
Layout fix. Same as current solution, but no need to replaceImageBackground
withImage
-
Option 2
UsezIndex
here:
function getReportWelcomeBackgroundImageViewStyle(isSmallScreenWidth) {
if (isSmallScreenWidth) {
return {
height: CONST.EMPTY_STATE_BACKGROUND.SMALL_SCREEN.VIEW_HEIGHT,
+ zIndex: -1,
};
}
return {
height: CONST.EMPTY_STATE_BACKGROUND.WIDE_SCREEN.VIEW_HEIGHT,
+ zIndex: -1,
};
}
@aimane-chnaif thank you for your suggestion. I'll check your approaches when I got a minute. Please don't merge this PR until I test @aimane-chnaif's suggestions. |
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.
Some updates to make things a bit nicer and fix a bug - let me know if everything makes sense!
e667329
Co-authored-by: Daniel Gale-Rosen <[email protected]>
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.
Looks good as long as it continues to test correctly! (@aimane-chnaif did you double check after these new changes?)
yes checked |
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.
Just a small comment from me but otherwise, looks great!
c75d245
✋ 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/MariaHCD in version: 1.2.79-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.79-0 🚀
|
Details
I couldn't pinpoint the root cause of the original issue. The issue seems to stem from the
ImageBackground
RN component. This PR replacesImageBackground
withImage
.In the original code,
ImageBackground
was contained inside thisView
component, which also acted as a filler to push thisView
component down.In the original code,
ImageBackground
overflew its parentView
component. This overflew part behaved differently depending on the platform. On laptop and mobile Safari and mobile Chrome, this overflew part covered theView
component which contains avatars and welcome texts.Instead of using
ImageBackground
, this PR usesImage
RN component with itsposition
set toabsolute
. By putting thisImage
before other sibling components in the code,Image
goes behind just like whatImageBackground
does.The
View
component which originally containedImageBackground
still needs to be there, as it also acts as a filler to position avatars and welcome texts correctly.Fixed Issues
$ #15237
Tests
Mobile Safari and Mobile Chrome
*You cannot zoom in or zoom out on NewDot on Chrome on Android device. You can use Chrome on iPhone to test zoom in and zoom out interactions.
Laptop Chrome and Safari, Desktop, Android and Safari Native
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Chrome
Safari
Mobile Web - Chrome
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.14.-.2023-02-22.at.15.30.01.mp4
Desktop
iOS
Android