Added tapable links to user's login information on DetailsPage #3870
Conversation
src/components/TappableCopy.js
Outdated
| styles.justifyContentCenter, | ||
| {right: -36, top: 0, bottom: 0}]} | ||
| > | ||
| <ReportActionContextMenuItem |
There was a problem hiding this comment.
Seems incorrect to re-use a ReportActionContextMenuItem here, no?
This is not inside the ReportActionContextMenu 🤔
There was a problem hiding this comment.
Yeah. I think we should rename it as we need exactly the same behaviour. Any name suggestions?
There was a problem hiding this comment.
i would vote to rename it to ContextMenuItem
|
@chiragsalian Updated. Thanks. |
There was a problem hiding this comment.
Missed a spot to update the copy https://github.com/parasharrajat/Expensify.cash/blob/4e1aae3da17f259206445a5f85b1d4e534df4930/src/pages/home/report/ReportActionContextMenu.js#L77
to ContextMenuItem.
Rest LGTM 👍
|
@chiragsalian All set. Updated. |
There was a problem hiding this comment.
LGTM, leaving final review/merge to you @marcaaron since you did leave a comment.
src/components/TappableCopy.js
Outdated
|
|
||
| const defaultProps = { | ||
| style: [], | ||
| type: undefined, |
There was a problem hiding this comment.
I don't quite understand this. When are we using this component and why would it not have a type? It looks like if there's no type we just return an empty View. 🤔
Can we maybe give this a better name than TappableCopy? It looks like it has a pretty specific purpose i.e. wrap anything (I don't see anything that limits this to "copy") in a Pressable and then do an email or phone link or generically copy the text to clipboard.
There was a problem hiding this comment.
Maybe something like CommunicationsLink
There was a problem hiding this comment.
There were two points to address so I'm reopening. Please allow reviewers to resolve comments.
|
@marcaaron Updated. thanks |
|
Updated |
|
Conflict resolved. Updated. |
| onPress={() => Clipboard.setString(props.value)} | ||
| /> | ||
| </View> | ||
| )} |
There was a problem hiding this comment.
Still don't get the checks for props.type. What is using this without a type? If there's no type we show props.children? We should not use it at all in that case.
There was a problem hiding this comment.
To make CommunicationLink uses cleaner I did this. Otherwise, we have to conditionally show the content wrapped in CommunicationLink which will duplicate a few lines on the main Component and This approach gives it a cleaner usage. https://github.com/parasharrajat/Expensify.cash/blob/a6814422a60fbba13d04e19f2807190a82450c2f/src/pages/DetailsPage.js#L95.
I will welcome suggestions to improve it.
There was a problem hiding this comment.
Ok, I am removing the confusing uses of Type. Instead, just use the conditional block here https://github.com/parasharrajat/Expensify.cash/blob/a6814422a60fbba13d04e19f2807190a82450c2f/src/pages/DetailsPage.js#L95
There was a problem hiding this comment.
Perfect thanks!
Otherwise, we have to conditionally show the content wrapped in CommunicationLink which will duplicate a few lines on the main Component and This approach gives it a cleaner usage.
I have a different opinion. To me it's not clear why we should use a component that might do nothing at all. It's as if we are calling a function with an optional argument that specifies whether we should call the function or not. In this case, it's clearer to just not call the function in the first place.
There was a problem hiding this comment.
Thanks for clarifying.
|
Updated. |
|
✋ 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 in version: 1.0.77-6🚀
|
|
🚀 Deployed to production in version: 1.0.79-4🚀
|
Details
Fixed Issues
$ Fixes #3476
Tests | QA Steps
Tested On
Screenshots
Web | Desktop
tap-w.mp4
Mobile Web
tap-m.mp4
iOS
Android
tap-a.mp4