-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: use pressable with press feedback #19391
feat: use pressable with press feedback #19391
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.
LGTM
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.
Overall LGTM 👌
@MonilBhavsar @rushatgabhane 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] |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-05-25.at.16.11.51.movMobile Web - ChromeScreen.Recording.2023-05-30.at.17.56.33.movMobile Web - SafariScreen.Recording.2023-05-30.at.17.57.05.movDesktopiOSScreen.Recording.2023-05-25.at.16.20.11.movAndroidWhatsApp.Video.2023-05-30.at.20.24.31.mp4 |
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.
@BeeMargarida tests well! Request two minor changes
/** The icon to display once the pressable is pressed */ | ||
iconChecked: PropTypes.func, | ||
|
||
/** If the component should be inline with text or not */ |
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 comment should describe when / why would we use this prop
containerStyles={styles.flexRow} | ||
text={props.isDelayButtonStateComplete ? props.tooltipTextChecked : props.tooltipText} | ||
> | ||
<Text |
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.
There is a console error caused by this line -
![image](https://private-user-images.githubusercontent.com/29673073/240887323-2c9fc4b0-9741-4a23-9353-b7b998654c46.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDE5NzcsIm5iZiI6MTcyNTYwMTY3NywicGF0aCI6Ii8yOTY3MzA3My8yNDA4ODczMjMtMmM5ZmM0YjAtOTc0MS00YTIzLTkzNTMtYjdiOTk4NjU0YzQ2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA1NDc1N1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTg3NTc1OTE0MWUyYjUzOGEzYWRmOWM4M2JlMzBmMDE1ZjIwNjBkYjc0NjRlMjU5ODI5Y2M2ODkyNzY4YTk5YjMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.UFRv4HslHrhnnnf0V2XF0z9VFZ88T8-LLUr08qVzZe4)
Failed prop type: Invalid prop
containerStyles
of typeobject
supplied toTooltip
, expected an array.
at Tooltip (webpack-internal:///./src/components/Tooltip/index.js:49:86)
at withWindowDimensions(Tooltip)
at div
at eval (webpack-internal:///./node_modules/@expensify/react-native-web/dist/exports/View/index.js:52:25)
at Pressable (webpack-internal:///./node_modules/@expensify/react-native-web/dist/exports/Pressable/index.js:40:24)
at PressableWithDelayToggle (webpack-internal:///./src/components/PressableWithDelayToggle.js:94:29)
@BeeMargarida On Android I'm getting this weird bug where the recovery codes overflow. Can you repro this? ![]() |
I'm currently OOO until 1st of June, but @thiagobrez will take it from me |
Hey! Looking into this now |
Hey @rushatgabhane!
These 2 are done, just pushed the fix ✅
This one is being fixed by this PR: #19260 Let me know what you think 🙌🏻 |
@rushatgabhane would you be able to take a look today, so we can merge this 🚀 |
@thiagobrez thank you! |
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.
@MonilBhavsar LGTM!
src/components/Button/index.js
Outdated
{textComponent} | ||
</View> | ||
{this.props.shouldShowRightIcon && ( | ||
<View style={styles.justifyContentCenter}> | ||
<View style={[styles.justifyContentCenter, styles.ml1, ...this.props.iconStyles]}> |
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.
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.
@MonilBhavsar Seems like the left icon styles were also being applied to the right icon. Fixed ✅
@thiagobrez the app crashes when we go to workspace > any workspace > bills. Could you please check. Thanks |
@MonilBhavsar Should be fixed now! |
Sorry for not noticing earlier. But while we're here, can we please also update the "Copy" button inside verify page, to be pressable with feedack. |
@MonilBhavsar Updated! Screen.Recording.2023-06-05.at.12.41.10.mov |
onPress={() => Clipboard.setString(props.account.twoFactorAuthSecretKey)} | ||
medium | ||
styles={[styles.button, styles.buttonMedium, styles.twoFactorAuthCodesButton]} |
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 the change!
I wonder if we can remove the minimum width property from styles.twoFactorAuthCodesButton
here as button looks wider. @shawnborton would like to hear your thoughts
![Screenshot 2023-06-05 at 5 18 47 PM](https://private-user-images.githubusercontent.com/32012005/243346076-d09c2f16-5001-46d8-bc3f-8560e9156392.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDE5NzcsIm5iZiI6MTcyNTYwMTY3NywicGF0aCI6Ii8zMjAxMjAwNS8yNDMzNDYwNzYtZDA5YzJmMTYtNTAwMS00NmQ4LWJjM2YtODU2MGU5MTU2MzkyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA1NDc1N1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMxMWJjZDc3NDc1OTc5NzVkMGUzOGU2NzExODdhZGI5ZmJhYzIyNzkyMzM3MTljNGIwMjljZjRlZDAxYjVmMGUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.g49YgkJw46-uipMLLmCERbQEzhsvAaiNF3UVc4Ex8Uw)
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.
I suppose we can get rid of it here, yeah.
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! @BeeMargarida can we please remove styles.twoFactorAuthCodesButton. Thanks!
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.
@MonilBhavsar Done!
![image](https://private-user-images.githubusercontent.com/25725586/243390685-198129ad-3cee-4488-85e9-ec898b3e09b0.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2MDE5NzcsIm5iZiI6MTcyNTYwMTY3NywicGF0aCI6Ii8yNTcyNTU4Ni8yNDMzOTA2ODUtMTk4MTI5YWQtM2NlZS00NDg4LTg1ZTktZWM4OThiM2UwOWIwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA1NDc1N1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWZkNDk5Njg0NjUyMTA2MzM5NDg4NzFmYmE1OWJjODNkN2Q1Y2M4YmEwZWZlOTJkMWZlOTM2Y2IzN2Q3ZDY2ZTAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.3Wx7lEJCW439mTAFWY8I0P9L-Ni7CxWtCf6OCvxC-LE)
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.
@MonilBhavsar @BeeMargarida I believe this minWidth was there to keep the width of these 2 buttons consistent (per design). Maybe we could remove only from the copy button, not from the whole style definition
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.
Oops, just saw that Ana did exactly that. Ignore me!
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! 🚀
✋ 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/MonilBhavsar in version: 1.3.24-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed, props.isDelayButtonStateComplete))} | ||
style={props.iconStyles} | ||
width={variables.iconSizeSmall} | ||
height={variables.iconSizeSmall} |
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.
Hi, we forgot to pass props.inline
to <Icon />
causing #22259
onPress={this.copyToClipboard} | ||
style={[styles.flexRow, styles.cursorPointer]} | ||
suppressHighlighting |
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.
Coming from #24591
We are missing the prop suppressHighlighting
if the PressableWithDelayToggle
uses the Text
component.
Details
withDelayToggleButtonState
and makes the logic of showing a checkmark icon and different tooltip text once the pressable was pressed.inline
prop istrue
, it usesText
as a wrapper instead ofPressable
, since there are some limitation on RN side regarding that ([SO](https://stackoverflow.com/questions/66590167/vertically-align-pressable-inside-a-text-component, RN issue 1, RN issue 2).CodesPage
buttonsCopyTextToClipboard
component to use the new component, since it's the same logicNOTE: the icon in Settings -> Profile -> Contact Method is not being displayed in Android. I checked in
main
and asked other people if they were experiencing the same, and it seems it's a problem. However, it's not related with this change.Fixed Issues
$ #19180
#19180 (comment)
Tests
Offline tests
Not relevant.
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
web_tiny.mp4
Mobile Web - Chrome
mChrome.mov
Mobile Web - Safari
mSafari.mov
Desktop
desktop.mov
iOS
ios_tiny.mp4
Android
android.mov