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 for: Web - Triple clicking an edited message select’s the (edited) label along with the actual message #17704

Merged
merged 13 commits into from
May 2, 2023
Merged

Fix for: Web - Triple clicking an edited message select’s the (edited) label along with the actual message #17704

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0951986
Mirror text layout from EditedRenderer
Ollyws Apr 20, 2023
861fc7c
Added dInlineFlex style
Ollyws Apr 20, 2023
18274bd
Add alignItemsBaseline,dInlineFlex to edited label
Ollyws Apr 20, 2023
93f3662
Moved MiniReportActionContextMenu to top of view
Ollyws Apr 20, 2023
45646d5
Removed userSelectNone from emoji text
Ollyws Apr 20, 2023
2619a9b
Merge branch 'main' into fix-15194
Ollyws Apr 21, 2023
db83d74
Added dInlineFlex index/index.native
Ollyws Apr 22, 2023
06b6454
Merge branch 'main' into fix-15194
Ollyws Apr 28, 2023
fcf4630
Added editedLabelStyles
Ollyws May 1, 2023
a994917
Revert remove userSelectNone
Ollyws May 2, 2023
133d4d5
Revert move MiniReportActionContextMenu
Ollyws May 2, 2023
7633f88
Move MiniReportActionContextMenu to top of view
Ollyws May 2, 2023
44bbe3c
Added userSelectNone to space Text
Ollyws May 2, 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
1 change: 1 addition & 0 deletions src/components/HTMLEngineProvider/HTMLRenderers/EditedRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const EditedRenderer = (props) => {
{...defaultRendererProps}
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={[styles.alignItemsBaseline, styles.dInlineFlex]}
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this is a Text Component where we are applying inline flex.

Copy link
Contributor Author

@Ollyws Ollyws Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat Yes, is this an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet.

>
{/* Native devices do not support margin between nested text */}
<Text style={styles.w1}>{' '}</Text>
Expand Down
4 changes: 3 additions & 1 deletion src/pages/home/report/ReportActionItemFragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,10 @@ const ReportActionItemFragment = (props) => {
<Text
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={[styles.alignItemsBaseline, styles.dInlineFlex]}
>
{` ${props.translate('reportActionCompose.edited')}`}
<Text style={styles.w1}>{' '}</Text>
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member

@parasharrajat parasharrajat Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see same as EditedRenderer. Correct?

Copy link
Contributor Author

@Ollyws Ollyws Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat This is necessary for our solution to work on Firefox and also fixes this problem with overflow.
Yes, correct the same code as EditedRenderer.

Copy link
Member

Choose a reason for hiding this comment

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

How does this work?

Copy link
Contributor Author

@Ollyws Ollyws Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the way Firefox deals with selection using flex, having a <span> between the two pieces of text stops them from both being selected at once.
It's a similar principle that causes them to break onto a second line properly.

Copy link
Member

@parasharrajat parasharrajat May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This has a drawback where the edited label does not start from the start of the line.
Screenshot 2023-05-01 at 8 55 35 PM

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts?

Copy link
Contributor Author

@Ollyws Ollyws May 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat I'm having a look at it, but I'm not sure there is any easy solution. Do you think it is a big issue?
We could do something like setting the space width to 0px and adding margin-right on the preceding text, but that changes where the preceding text breaks.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think @marcochavezf?

Copy link
Member

Choose a reason for hiding this comment

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

Another issue with this change is that this space is selectable via triple click.

  • triple click on only the space and notice that it is selectable.

Can we just replace it with a margin?

Copy link
Contributor Author

@Ollyws Ollyws May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@parasharrajat We could just add userSelectNone to prevent selection on triple click. Note: in this PR it will also copy the space but when #18228 is merged it shouldn't.
I'm concerned how removing the space would affect the original intention of adding it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's add that.

{props.translate('reportActionCompose.edited')}
</Text>
)}
</Text>
Expand Down
4 changes: 4 additions & 0 deletions src/styles/utilities/display.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@ export default {
dInline: {
display: 'inline',
},

dInlineFlex: {
display: 'inline-flex',
},
};