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 4 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 @@ -48,6 +49,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(
Str.isValidEmailMarkdown(props.displayName) ? ContextMenuActions.CONTEXT_MENU_TYPES.EMAIL : ContextMenuActions.CONTEXT_MENU_TYPES.LINK,
Expand Down
5 changes: 1 addition & 4 deletions src/components/HTMLEngineProvider/defaultViewProps/index.web.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import styles from '../../../styles/styles';

export default {
// For web platform default to block display. Using flex on root view will force all
// child elements to be block elements even when they have display inline added to them.
// This will affect elements like <a> which are inline by default.
style: [styles.dBlock, styles.userSelectText],
style: [styles.userSelectText],
Copy link
Member

Choose a reason for hiding this comment

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

I see we are reverting to this change. But where are we fixing the original issue the change was fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By passing the cursorDefault style in PressableWithSecondaryInteraction in BaseAnchorForCommentsOnly.

};
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,14 +77,14 @@ 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}
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
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