-
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
fix: check canUseTouchScreen on each render #24356
fix: check canUseTouchScreen on each render #24356
Conversation
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.
This change makes sense to me. cc: @bernhardoj @kidroca since you both were discussing this in Slack here
@hannojg is testing only on web enough for this PR? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-08-10.at.1.31.55.PM.movScreen.Recording.2023-08-10.at.1.28.24.PM.movMobile Web - ChromeScreen.Recording.2023-08-10.at.4.55.47.PM.movMobile Web - SafariDesktopScreen.Recording.2023-08-10.at.5.00.00.PM.moviOSAndroidScreen.Recording.2023-08-10.at.4.53.52.PM.mov |
@MariaHCD testing on at least one touch screen based device as well would be good(android or iOS) |
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.
This change makes sense to me. cc: @bernhardoj @kidroca since you both were discussing this in Slack here
LGTM!
@@ -27,6 +26,7 @@ function AttachmentCarousel({report, reportActions, source, onNavigate}) { | |||
const scrollRef = useRef(null); | |||
|
|||
const {windowWidth, isSmallScreenWidth} = useWindowDimensions(); | |||
const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); |
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.
One minor concern is that we currently can't trigger a re-render when the value from DeviceCapabilities.canUseTouchScreen()
changes. This means we're dependent on external factors to trigger a re-render of AttachmentCarousel
before it acknowledges any change in the canUseTouchScreen()
value.
However, given the context of this specific use-case, this behavior seems acceptable.
@hannojg Okay, I'll test on native Android and mWeb Android. Could you test on iOS and add to your checklist? |
Actually, we still have one case where the
However, looking at the commit message, this is done to fix arrows not showing on the web 🤔, but from my testing the arrow is showing fine on the web/mWeb. |
@MariaHCD tested on iOS and added iOS screenshot to the checklist 👍 |
@bernhardoj You mean when you revert the changes from the commit (so that canUseTouchScreen is inside the FC) you can still see the arrows? |
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 fixing!
I see them its just oddly formatted. |
Triggered a build so we can test on the device more easily |
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.
This looks good to me and tests well!
@hannojg, is there any follow up regarding the arrow keys that @bernhardoj brought up here?
Not sure, I want @chrispader to double check since its his original PR! |
I added a |
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.
That makes sense to me! We've just got some lint errors now.
Sorry, done 👍 |
I have a question. If we are updating |
Yes, as it currently stands, it seems we've opted for a polling approach. We're checking
No offense, but I'm curious about why we chose to create a
|
src/hooks/useCanUseTouchScreen.js
Outdated
const [canUseTouchScreen, setCanUseTouchScreen] = useState(DeviceCapabilities.canUseTouchScreen()); | ||
|
||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
useEffect(() => { | ||
const newCanUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | ||
if (canUseTouchScreen === newCanUseTouchScreen) return; | ||
setCanUseTouchScreen(); | ||
}); | ||
|
||
return canUseTouchScreen; |
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.
You can achieve the same outcome with a more straightforward approach:
const [canUseTouchScreen, setCanUseTouchScreen] = useState(DeviceCapabilities.canUseTouchScreen()); | |
// eslint-disable-next-line react-hooks/exhaustive-deps | |
useEffect(() => { | |
const newCanUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | |
if (canUseTouchScreen === newCanUseTouchScreen) return; | |
setCanUseTouchScreen(); | |
}); | |
return canUseTouchScreen; | |
return DeviceCapabilities.canUseTouchScreen(); |
Here's why this alternative is equivalent:
- The current implementation uses a
useEffect
that runs logic on each render (since there's no dependencies array). - In such a scenario, you could simply execute the logic in the hook body, eliminating the need for a
useEffect
andstate
Co-authored-by: Bernhard Owen Josephus <50919443+bernhardoj@users.noreply.github.com>
Co-authored-by: Maria D'Costa <mariahcdcosta@gmail.com>
Yes, you're right. This was a silly mistake on my side... |
Just holding on a final @kidroca approval |
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.
LGTM!
✋ 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.3.70-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀
|
Details
When implementing:
a change in behaviour got introduced where in the AttachmentCarousel before the
DeviceCapabilities.canUseTouchScreen
was checked every render, but after the PR it was only checked once on app start.This reverts back to the old behaviour where its checked every render.
Fixed Issues
$ https://expensify.slack.com/archives/C049HHMV9SM/p1691474174723719
PROPOSAL:
Tests
Offline 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 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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android