Skip to content
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.

Sign up for GitHub

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

fix: offline deleted heading styles and link padding #19545

Merged
merged 6 commits into from
Jun 8, 2023
Merged

fix: offline deleted heading styles and link padding #19545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1b3f25
fix: offline deleted heading styles and link padding
Nikhil-Vats May 23, 2023
0519582
fix issue with display inline being passed to native
Nikhil-Vats May 24, 2023
43ae959
Add support for parsing styles as function
Nikhil-Vats May 25, 2023
79a9cd5
Use StyleUtils for fontSize
Nikhil-Vats May 30, 2023
69dc50a
Merge branch 'main' into issue/18658_fix_deleted_heading_style_in_off…
Nikhil-Vats Jun 8, 2023
8df3eea
Fix issue in header links after merge conflicts
Nikhil-Vats Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as ContextMenuActions from '../../pages/home/report/ContextMenu/Context
import Tooltip from '../Tooltip';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
import {propTypes as anchorForCommentsOnlyPropTypes, defaultProps as anchorForCommentsOnlyDefaultProps} from './anchorForCommentsOnlyPropTypes';

Expand Down Expand Up @@ -53,6 +54,7 @@ const BaseAnchorForCommentsOnly = (props) => {
return (
<PressableWithSecondaryInteraction
inline
style={[styles.cursorDefault, StyleUtils.getFontSizeStyle(props.style.fontSize)]}
Copy link
Contributor

@youssef-lr youssef-lr Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we added this? Removing this it fixes this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is caused by this line but reverting it is not the solution because we were adding the pointer cursor not to the link but to its parent which showed the pointer at places around the link -

Screen.Recording.2023-06-13.at.9.46.29.PM.mov

The root cause for this issue is that for some reasons links like https://staging.new.expensify.com/random_text_here are rendered in div rather than a which is wrong.

Screen.Recording.2023-06-13.at.9.48.01.PM.mov

So, we need to figure out why links are rendered in div and change it to a to fix it the right way.

Copy link
Member

@parasharrajat parasharrajat Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal links need special handling before opening them then they are opened via onPress handlers. We don't want them to open immediately so a tag is disabled on them. Only when href is passed AnchorRenderer are rendered as a. THis was the issue I was mentioning #19545 (comment) when I said that this will not work for the #16526. For the same reason, I didn't select a cursor-based solution on that solution. You can see that it is presented as a proposal on that.

Unfortunately, this didn't come to my mind before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for weighing in @parasharrajat. I think helps us further to figure out a fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mollfpr @cristipaval This PR was reverted because of the special handling of internal links explained by @parasharrajat above.

I found a solution -
basically for internal links since they are rendered as div we can explicitly set cursor to pointer on line 71. For normal links it is added by default so the only behaviour we are changing is for internal links by adding pointer cursor -
(Text below uses div for internal links and a for other/normal links)

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js

Lines 68 to 71 in 6e8435e

<Tooltip text={props.href}>
<Text
ref={(el) => (linkRef = el)}
style={StyleSheet.flatten([props.style, defaultTextStyle])}

Need to change line 71 to -

style={StyleSheet.flatten([props.style, defaultTextStyle, styles.cursorPointer])}

This would solve all the related issues without regression. I tested on local all issues that were related to my issue like #18658, #17488, #16526, are fixed.

Result -

Screen.Recording.2023-06-13.at.11.01.48.PM.mov

onSecondaryInteraction={(event) => {
ReportActionContextMenu.showContextMenu(
isEmail ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
Expand Down
16 changes: 6 additions & 10 deletions src/components/AnchorForCommentsOnly/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,16 @@
import React from 'react';
import {View} from 'react-native';
import * as anchorForCommentsOnlyPropTypes from './anchorForCommentsOnlyPropTypes';
import BaseAnchorForCommentsOnly from './BaseAnchorForCommentsOnly';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import ControlSelection from '../../libs/ControlSelection';
import styles from '../../styles/styles';

const AnchorForCommentsOnly = (props) => (
<View style={styles.dInline}>
<BaseAnchorForCommentsOnly
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
onPressOut={() => ControlSelection.unblock()}
/>
</View>
<BaseAnchorForCommentsOnly
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
onPressIn={() => DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
onPressOut={() => ControlSelection.unblock()}
/>
);

AnchorForCommentsOnly.propTypes = anchorForCommentsOnlyPropTypes.propTypes;
Expand Down
2 changes: 1 addition & 1 deletion src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const ImageRenderer = (props) => {
>
{({show}) => (
<PressableWithoutFocus
style={styles.noOutline}
styles={styles.noOutline}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there was a typo here since the name of prop for PressableWithoutFocus is styles not style.

App/src/components/PressableWithoutFocus.js

Line 17 in 213ba55

styles: PropTypes.arrayOf(PropTypes.object),

I noticed this while reviewing the PR for #19717 so I am fixing it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this console error:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting fixed in one of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parasharrajat can you link the PR if possible?
Because if that’s a big one and will take time to be merged, I can raise a PR and fix this sooner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mollfpr @cristipaval
styles has to be an array here and I passed an object.

I made this typo while fixing an issue created in a related PR. Shall I create a new PR for this so that it is fixed before QA testing? Otherwise if the other PR that @parasharrajat is talking about takes a lot of time then this PR will be blocked unnecessarily.

Copy link
Member

@parasharrajat parasharrajat Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to drop another PR to fix this. Please follow this https://github.com/Expensify/App/pull/20202/files#r1224269258

onPress={show}
onLongPress={(event) => showContextMenuForReport(event, anchor, report.reportID, action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
>
Expand Down
2 changes: 1 addition & 1 deletion src/components/PressableWithSecondaryInteraction/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ class PressableWithSecondaryInteraction extends Component {
// On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text.
return (
<Pressable
style={StyleUtils.combineStyles(this.props.inline ? styles.dInline : this.props.style)}
onPressIn={this.props.onPressIn}
onLongPress={this.props.onSecondaryInteraction ? this.executeSecondaryInteraction : undefined}
activeOpacity={this.props.activeOpacity}
Expand All @@ -86,6 +85,7 @@ class PressableWithSecondaryInteraction extends Component {
ref={(el) => (this.pressableRef = el)}
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultPressableProps}
style={(state) => [StyleUtils.parseStyleFromFunction(this.props.style, state), ...[this.props.inline && styles.dInline]]}
>
{this.props.children}
</Pressable>
Expand Down