-
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
feat: migrate PressableWithSecondaryInteraction to function component #25398
feat: migrate PressableWithSecondaryInteraction to function component #25398
Conversation
@0xmiroslav @marcaaron 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.
Looks good
Updated 👍 |
@0xmiroslav working on the checklist? Lmk when this is ready for me 🙇♂️ |
@0xmiroslav Any update? |
I'll be OOO this week, but someone from Callstack will pick this up if necessary |
Please add any test case affected by this PR in QA step |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
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!
I believe we are on a merge freeze at the moment so I am holding for a bit - unless this fixes a critical bug or something. |
…eraction-to-functional
PR updated with main, ready for merge |
@0xmiroslav @marcaaron Pinging about this, so that it does not get forgotten |
Please also re-test after merging main |
Re-tested, seems to be working fine 👍 |
@0xmiroslav Hi, any update? Is anything blocking this? |
LGTM! Already approved by me. |
@marcaaron looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@BeeMargarida Seems like our PR caused console error in Hoverable component. Can you please check? Repro step: hover LHN option items Screen.Recording.2023-09-16.at.8.58.55.AM.mov |
Also, emoji reaction right click not working Screen.Recording.2023-09-16.at.9.04.13.AM.mov |
e.preventDefault(); | ||
if (forwardedRef) { | ||
if (_.isFunction(forwardedRef)) { | ||
forwardedRef(pressableRef); |
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.
forwardedRef(pressableRef); | |
forwardedRef(pressableRef.current); |
I think this will fix both issues
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.
raised PR for this fix: #27576
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.71-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.71-12 🚀
|
Details
Refactored
PressableWithSecondaryInteraction
(web) from class to function componentFixed Issues
$ #16192
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
safari_t.mp4
chrome_t.mp4
Mobile Web - Chrome
mchrome_t.mp4
Mobile Web - Safari
msafari_t.mp4
Desktop
desktop_t.mp4
iOS
ios_t.mp4
Android
android_t.mp4