Closed bernhardoj closed 1 week ago
@mananjadhav 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]
@mananjadhav I tested a multi-page PDF and I can't scroll it on Android, basically this PR will reraise https://github.com/Expensify/App/issues/20867 again. https://github.com/Expensify/App/issues/20867 is the one that causes our issue here. The root cause is explained on the proposal there, but in short, it's because our Pressable component when disabled doesn't really disable the Pressable, but just pass an undefined event handler to it.
I think to solve both issues, we should have a new prop for Pressable called something like, fullDisabled
and use it to set the disabled
props of BaseGenericPressable. What do you think?
when disabled doesn't really disable the Pressable, but just pass an undefined event handler to it.
@bernhardoj I don't completely follow what you mean? You mean in our PR or the props in general? I am not sure if we really need fullDisabled
. Can you rephrase what you mean? May be we can then come up with an alternative.
cc - @blimpich
@mananjadhav that's how the disabled
props of our custom Pressable currently works as shown in this code.
https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/components/Pressable/GenericPressable/BaseGenericPressable.tsx#L139-L143
There is no disabled={disabled}
. We are just passing an undefined callback, so the pressable responder is still working. The only way to disable the responder is by setting the RN Pressable disabled
to true
.
and what happens if we actually add disabled: true
in here.
It will still be the same since we take out the disabled
props from the ...rest
props.
Okay I am okay having a fullDisabled
prop. @blimpich Any thoughts?
Fine by me 👍
@bernhardoj Can we work on the fix?
@mananjadhav updated
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.:hand: 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/chiragsalian in version: 1.4.76-0 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.76-7 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
Details
The blink happens because we change the component tree when the pdf loads completes which will recreate the pdf component again. This PR fix it.
Fixed Issues
$ https://github.com/Expensify/App/issues/42103 PROPOSAL: https://github.com/Expensify/App/issues/42103#issuecomment-2110325553
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: send a pdf attachment
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
https://github.com/Expensify/App/assets/50919443/cd78b219-5bd9-4599-9be0-f5c1c5578991Android: mWeb Chrome
https://github.com/Expensify/App/assets/50919443/ffc5f609-2a47-45ce-aa7f-a99813fe9b1fiOS: Native
https://github.com/Expensify/App/assets/50919443/a5c56d1f-ab3e-4673-9a80-ffdcff5fa1e3iOS: mWeb Safari
https://github.com/Expensify/App/assets/50919443/3f683de7-5ca2-4638-8842-2e0e48200af4MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/50919443/1903adfa-4970-440d-994f-0c73d3e5aac4MacOS: Desktop
https://github.com/Expensify/App/assets/50919443/6e9ae2b5-6f0a-48b8-b15a-6f41ff77d52a