Show full link when hovering over hyperlink #8815
Conversation
| * @returns {String} | ||
| */ | ||
| getPopoverDescription() { | ||
| const isNative = Platform.OS === 'ios' || Platform.OS === 'android'; |
There was a problem hiding this comment.
This is an incorrect way to do platform detection. You can read more about that here: https://github.com/Expensify/App#platform-specific-file-extensions
There was a problem hiding this comment.
This is an incorrect way to do platform detection. You can read more about that here: https://github.com/Expensify/App#platform-specific-file-extensions
I'm unsure on how to divide this logic into platform-specific files.
How about implementing isNative() method in getOperatingSystem, which in index.native.js would just return true and index.js would return false
I uploaded a video of desktop to the PR description for you.
Thanks!
There was a problem hiding this comment.
You could take a look at some of the existing platform-specific files to get some ideas, but without digging too deep into this code, my initial ideas are:
- Create a module like
src/pages/home/report/ContextMenu/ContextMenuUtils/index.jswhich would have a method that acceptsthis.state.typeandthis.state.selectionas parameters. - Create a new platform-specific component that wraps
<BaseReportActionContextMenu>and provides the necessary logic
| this.props.href, | ||
| lodashGet(linkRef, 'current'), | ||
| ); | ||
| <Tooltip text={!Str.isValidEmail(this.props.displayName) && this.props.href}> |
There was a problem hiding this comment.
Let's move this tooltip on to the Text Node.
src/components/MenuItem.js
Outdated
| @@ -101,7 +101,7 @@ const MenuItem = props => ( | |||
| > | |||
| {props.title} | |||
| </Text> | |||
| {props.description && ( | |||
| {!!props.description && ( | |||
There was a problem hiding this comment.
| {!!props.description && ( | |
| {Boolean(props.description) && ( |
| getPopoverDescription() { | ||
| const isNative = Platform.OS === 'ios' || Platform.OS === 'android'; | ||
| if ((isNative || this.props.isSmallScreenWidth) && this.state.type === ContextMenuActions.CONTEXT_MENU_TYPES.LINK) { | ||
| return this.state.selection; | ||
| } | ||
| return ''; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a good way of doing this. I don't agree with this change. How the whole menu description goes to each item's description? Can you think of a way to do this with ContextMenuActions?
| * @returns {String} | ||
| */ | ||
| getPopoverDescription() { | ||
| const isNative = Platform.OS === 'ios' || Platform.OS === 'android'; |
|
Updated |
| @@ -293,6 +293,7 @@ class PopoverReportActionContextMenu extends React.Component { | |||
| reportID={this.state.reportID} | |||
| reportAction={this.state.reportAction} | |||
| draftMessage={this.state.reportActionDraftMessage} | |||
| selection={this.state.selection} | |||
There was a problem hiding this comment.
Same comment here which I used for the description key.
How the whole menu description goes to each item's description? Can you think of a way to do this with ContextMenuActions?
We don't need this selection at all.
| @@ -49,6 +53,7 @@ class BaseReportActionContextMenu extends React.Component { | |||
| draftMessage: this.props.draftMessage, | |||
| selection: this.props.selection, | |||
| })} | |||
| description={ContextMenuUtils.getPopoverDescription(this.props.type, this.props.selection, this.props.isSmallScreenWidth)} | |||
There was a problem hiding this comment.
Did you notice how most of the props are coming from contextAction? This change is breaking the existing pattern. This is a side-effect.
Let's discuss the alternative first before making the change. Consider that the only way to present data here is to get it from ContextMenuActions. Now, what can be done to solve this problem?
Look at this
. How are we picking between actions?Can we show a new item here dynamically?
What if we have to show different descriptions for different menu items. Will your solution work in an ad-hoc way?
There was a problem hiding this comment.
Consider that the only way to present data here is to get it from
ContextMenuActions. Now, what can be done to solve this problem?
Can we show a new item here dynamically?
What if we have to show different descriptions for different menu items. Will your solution work in an ad-hoc way?
We could
- Add
getDescriptionmethod here
{
textTranslateKey: 'reportActionContextMenu.copyURLToClipboard',
icon: Expensicons.Clipboard,
successTextTranslateKey: 'reportActionContextMenu.copied',
successIcon: Expensicons.Checkmark,
shouldShow: type => type === CONTEXT_MENU_TYPES.LINK,
onPress: (closePopover, {selection}) => {
Clipboard.setString(selection);
hideContextMenu(true, ReportActionComposeFocusManager.focus);
},
getDescription: ({selection}) => selection,
},- Call it from
BaseReportActionContextMenuhere
description={contextAction.getDescription && contextAction.getDescription({selection: this.props.selection})}And if we will need to show different description for, say, CONTEXT_MENU_TYPES.EMAIL, we could add a getDescription method to it too
getDescription: ({otherParam}) => otherParam,Then step 2 would change to
description={contextAction.getDescription && contextAction.getDescription({selection: this.props.selection, otherParam: 'lorem ipsum'})}Would this work as an alternative?
There was a problem hiding this comment.
Sounds good to me. How will you control to hide the description on Desktop | Web?
There was a problem hiding this comment.
Sounds good to me. How will you control to hide the description on Desktop | Web?
I would implement shouldShowDescription method in src/pages/home/report/ContextMenu/ContextMenuUtils, for native it would just return true, and for web it would look like this:
function shouldShowDescription(isSmallScreenWidth) {
return isSmallScreenWidth;
}But I'm not sure where to call it
We could call it in getDescription method for each action type individually, like this:
getDescription: ({selection}) => shouldShowDescription() ? selection : '',But i think it makes more sense to call it for all actions in BaseReportActionContextMenu?
description={(contextAction.getDescription && ContextMenuUtils.shouldShowDescription(props.isSmallScreenWidth)) && contextAction.getDescription({selection: this.props.selection})}There was a problem hiding this comment.
What if we pass the isSmallScreenWidth directly to the description callback and decide to show the value here.
getDescription: ({selection}) => shouldShowDescription() ? selection : '',
There was a problem hiding this comment.
What if we pass the isSmallScreenWidth directly to the description callback and decide to show the value here.
getDescription: ({selection}) => shouldShowDescription() ? selection : '',
We need to check for native platform, so it would look like this:
getDescription: ({selection, isSmallScreenWidth}) => ContextMenuUtils.shouldShowDescription(isSmallScreenWidth) ? selection : '',|
Updated |
| function shouldShowDescription(isSmallScreenWidth) { | ||
| return isSmallScreenWidth; | ||
| } | ||
|
|
||
| export { | ||
| shouldShowDescription, | ||
| }; |
There was a problem hiding this comment.
| function shouldShowDescription(isSmallScreenWidth) { | |
| return isSmallScreenWidth; | |
| } | |
| export { | |
| shouldShowDescription, | |
| }; | |
| function getPopoverDescription(selection, isSmallScreenWidth) { | |
| return isSmallScreenWidth ? selection : ''; | |
| } | |
| export { | |
| getPopoverDescription, | |
| }; |
There was a problem hiding this comment.
Change the method name to be more descriptive and perform slightly different logic.
| function shouldShowDescription() { | ||
| return true; | ||
| } | ||
|
|
||
| export { | ||
| shouldShowDescription, | ||
| }; |
There was a problem hiding this comment.
| function shouldShowDescription() { | |
| return true; | |
| } | |
| export { | |
| shouldShowDescription, | |
| }; | |
| function getPopoverDescription() { | |
| return ''; | |
| } | |
| export { | |
| getPopoverDescription, | |
| }; |
| @@ -49,6 +52,7 @@ class BaseReportActionContextMenu extends React.Component { | |||
| draftMessage: this.props.draftMessage, | |||
| selection: this.props.selection, | |||
| })} | |||
| description={contextAction.getDescription ? contextAction.getDescription({selection: this.props.selection, isSmallScreenWidth: this.props.isSmallScreenWidth}) : ''} | |||
There was a problem hiding this comment.
| description={contextAction.getDescription ? contextAction.getDescription({selection: this.props.selection, isSmallScreenWidth: this.props.isSmallScreenWidth}) : ''} | |
| description={contextAction.getDescription ? contextAction.getDescription(this.props.selection, this.props.isSmallScreenWidth) : ''} |
There was a problem hiding this comment.
No need to pass those arguments as an object.
| @@ -62,6 +63,7 @@ export default [ | |||
| Clipboard.setString(selection); | |||
| hideContextMenu(true, ReportActionComposeFocusManager.focus); | |||
| }, | |||
| getDescription: ({selection, isSmallScreenWidth}) => (ContextMenuUtils.shouldShowDescription(isSmallScreenWidth) ? selection : ''), | |||
There was a problem hiding this comment.
| getDescription: ({selection, isSmallScreenWidth}) => (ContextMenuUtils.shouldShowDescription(isSmallScreenWidth) ? selection : ''), | |
| getDescription: ContextMenuUtils.getPopoverDescription, |
There was a problem hiding this comment.
This cleans up really well now and it's just passing a reference
Verified it working on all platforms and updated the PR |
| @@ -49,6 +52,7 @@ class BaseReportActionContextMenu extends React.Component { | |||
| draftMessage: this.props.draftMessage, | |||
| selection: this.props.selection, | |||
| })} | |||
| description={contextAction.getDescription ? contextAction.getDescription(this.props.selection, this.props.isSmallScreenWidth) : ''} | |||
There was a problem hiding this comment.
| description={contextAction.getDescription ? contextAction.getDescription(this.props.selection, this.props.isSmallScreenWidth) : ''} | |
| description={contextAction.getDescription(this.props.selection, this.props.isSmallScreenWidth)} |
Now noop (() => {}) the getDescription for all other actions.
There was a problem hiding this comment.
Yeah, I was trying to see if we could use something like _.result() or even https://github.com/Expensify/expensify-common/blob/main/lib/Func.jsx#L13 and neither of those are really what would be nice. Ideally it would be something where:
- If it's a function
- invoke the function with the given params
- if it's not a function
- return a default value
I had thought about a no-op, but didn't have too be of an opinion on adding it to all the other actions. The noop does make it a bit cleaner.
| * We should show popover description only on mWeb | ||
| * | ||
| * @param {String} selection | ||
| * @param {Boolean} isSmallScreenWidth | ||
| * @returns {Boolean} |
There was a problem hiding this comment.
JS docs need updated now in both modules (eg. @returns is wrong) and the method description should probably say something like:
The popover description will be an empty string on anything but mobile web because... (I'm not actually 100% why)
|
Updated |
There was a problem hiding this comment.
Please proactively test your changes. These findings tell me that you are not carefully testing.
On the web, when Content_menu is open. Tooltip covers it.
on MWeb, the link overflows the Popover menu.
On Native, the link is spread across multiple lines and overflowing.
|
@shawnborton Could you please help us finalize the UI for native? IMO, link should be limited to 2 lines and then clipped with ellipsis. |
Will do
I thought this is a separate issue, since it was present before this commit. For instance, when you right-click on username cinnamon-20220429-2.mp4But is we need to fix it, what is the intended behavior here? To show context menu on top of the tooltip? cinnamon-20220429-3.mp4
True, waiting for the design confirmation |
src/components/MenuItem.js
Outdated
| @@ -90,7 +90,7 @@ const MenuItem = props => ( | |||
| /> | |||
| </View> | |||
| )} | |||
| <View style={[styles.justifyContentCenter, styles.menuItemTextContainer]}> | |||
| <View style={[styles.justifyContentCenter, styles.menuItemTextContainer, styles.w100]}> | |||
There was a problem hiding this comment.
| <View style={[styles.justifyContentCenter, styles.menuItemTextContainer, styles.w100]}> | |
| <View style={[styles.justifyContentCenter, styles.menuItemTextContainer, styles.flex1]}> |
src/components/MenuItem.js
Outdated
| @@ -59,7 +59,7 @@ const MenuItem = props => ( | |||
| > | |||
| {({hovered, pressed}) => ( | |||
| <> | |||
| <View style={[styles.flexRow, styles.pointerEventsNone]}> | |||
| <View style={[styles.flexRow, styles.pointerEventsNone, styles.w100, styles.pr5]}> | |||
There was a problem hiding this comment.
This padding will break the UI.
| <View style={[styles.flexRow, styles.pointerEventsNone, styles.w100, styles.pr5]}> | |
| <View style={[styles.flexRow, styles.pointerEventsNone, styles.flex1]}> |
Works like a charm! |
Now only this is the problem. I think we can hide the tooltip as soon as the menu is visible.
It is not that simple. It will break many other things. I guess we can just close the tooltip when the dropdown opens. What alternatives do you suggest? |
We could do it like this:
const tooltipRef = React.createRef();
<Tooltip
text={Str.isValidEmail(this.props.displayName) ? '' : this.props.href}
ref={tooltipRef}
>
onSecondaryInteraction={
(event) => {
ReportActionContextMenu.showContextMenu(
Str.isValidEmail(this.props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
event,
this.props.href,
lodashGet(linkRef, 'current'),
undefined,
undefined,
undefined,
undefined,
undefined,
tooltipRef
);
}
}
if (tooltipRef.current) {
tooltipRef.current.hideTooltip();
}
function isPopoverVisible() {
if (!contextMenuRef.current) {
return;
}
return contextMenuRef.current.state.isPopoverVisible;
}
showTooltip() {
if (ReportActionContextMenu.isPopoverVisible()) {
return;
}It looks like this: cinnamon-20220501-1.mp4If this is the acceptable approach, there is one thing to consider. Do we hide only the tooltips that were added with this commit (when you hover over links) or for all tooltips in the app? If it's the first, we should probably add a prop (for example called |
|
It is much more complex than I thought @eVoloshchak.
Do you have any other suggestions other than hiding the tooltip? Another thing that we can do is to set fullscreen={true} On . Only drawback is that user has to close the modal before doing any other actions. But I feel that this will be fine as other apps also have this behavior I added this prop to allow direct interaction with the app even if the context menu is open. It closes the menu as soon as use trigger any other interaction.for example, the user can directly open the context menu on any other location if the context menu is already open. |
Apart from hiding it, the only alternative I can think of is moving the tooltip up when the ContextMenu opens (or opening the ContextMenu below the tooltip). But i think that would look kinda strange. And I think It would require a lot of changes, judging by the code used to calculate Tooltip and ContextMenu positions.
It works good imho. It's kind of intuitive, if user has opened the ContextMenu, it's their main focus |
Ok. Great. @eVoloshchak Could you please start a slack discussion for it? Please tag all reviewers in it. Let's see what others have to say about it. It can be helpful to show videos on the slack with or without fullscreen. |
Raised here. I'm not sure who you meant by 'all reviewers', so I tagged you and |
|
reviewers = PR reviewers. You did it correctly. |
|
@parasharrajat
This means we set
I'm guessing this is already being done in some other issue? |
|
Sounds like the max-width is being done already, so that's OK. I think I'm OK with merging this the way it is, but I would like to have you open up a new GH to explore making this work nicer with the context-menu. |
|
@eVoloshchak IMO, we should go with setting the fullscreen to true. It seems Shawn agrees to it. We can tackle this if anyone has an issue with this which they should not as the browser context menu also works the same way. One thing that we would want after changing to fullscreen is that right-clicking should not open the browser context menu when our menu is opened. But this is not part of this issue so you can leave this part. |
Should I open a new PR which sets it to |
|
I think it can be done in this one. It is a 1-liner change. Right? |
Sorry, my brain somehow confused "approved" with "merged" :) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
Details
If you have a message in chat containing hyperlink, there should be a tooltip displaying the full link address
Fixed Issues
#7844
Tests
Web/Desktop App: hower over hyperlink and verify that the tooltip containing full link address is shownmWeb/Native: long press on link hyperlink verify that full link address is displayed below theCopy URL to clipboardtitlePR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Web/Desktop App: hower over hyperlink and verify that the tooltip containing full link address is shownmWeb/Native: long press on hyperlink and verify that full link address is displayed below theCopy URL to clipboardtitleScreenshots
Web
cinnamon-20220428-1.mp4
Mobile Web
22-04-28-17-09-39.mp4
Note:I had to apply a temporary workaround to record this video, it's impossible to test this in prod due to #8311Desktop
I'm unable to launch the desktop app due to this bug. It is not present if you downgrade to certain node and npm versions, but since I use MacinCloud, I'm unable to do so. So if someone with a mac could test it and upload a video, I would highly appreciate it.
iOS
Screen.Recording.2022-04-28.at.4.24.59.PM.mp4
Android
22-04-28-17-11-10.mp4