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 Edited Line Through Alignment #17781

Merged
merged 16 commits into from
May 26, 2023
Merged

Fix Edited Line Through Alignment #17781

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f9d3ec3
Fix line through alignment
pubudu-ranasinghe Apr 18, 2023
2e56bd7
Fix line through alignment for markdown
pubudu-ranasinghe Apr 18, 2023
11a3777
Merge branch 'Expensify:main' into fix-17181-offline-edited-line-thro…
pubudu-ranasinghe Apr 19, 2023
4fffba5
Merge branch 'Expensify:main' into fix-17181-offline-edited-line-thro…
pubudu-ranasinghe Apr 20, 2023
32627e7
Limit styling issue fix to web only
pubudu-ranasinghe Apr 21, 2023
8ea952e
Merge branch 'fix-17181-offline-edited-line-through-styling' of githu…
pubudu-ranasinghe Apr 21, 2023
ea026be
Merge branch 'Expensify:main' into fix-17181-offline-edited-line-thro…
pubudu-ranasinghe Apr 23, 2023
74ef8c9
Remove platform specific code
pubudu-ranasinghe Apr 24, 2023
63772da
Merge branch 'Expensify:main' into fix-17181-offline-edited-line-thro…
pubudu-ranasinghe Apr 25, 2023
c5bf385
Merge branch 'Expensify:main' into fix-17181-offline-edited-line-thro…
pubudu-ranasinghe Apr 27, 2023
d977935
Merge branch 'main' into fix-17181-offline-edited-line-through-styling
pubudu-ranasinghe May 3, 2023
5e9687a
Fix edited label line through alignment
pubudu-ranasinghe May 3, 2023
83a0173
Cleanup code
pubudu-ranasinghe May 3, 2023
6e0b118
Merge branch 'main' into fix-17181-offline-edited-line-through-styling
pubudu-ranasinghe May 22, 2023
4d4d79a
Fix line through for normal messages
pubudu-ranasinghe May 22, 2023
a4f096e
Use boolean parsing and fix formatting
pubudu-ranasinghe May 25, 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
6 changes: 6 additions & 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 @@ -6,6 +6,8 @@ import Text from '../../Text';
import variables from '../../../styles/variables';
import themeColors from '../../../styles/themes/default';
import styles from '../../../styles/styles';
import CONST from '../../../CONST';
import getPlatform from '../../../libs/getPlatform';

const propTypes = {
...htmlRendererPropTypes,
Expand All @@ -14,12 +16,16 @@ const propTypes = {

const EditedRenderer = (props) => {
const defaultRendererProps = _.omit(props, ['TDefaultRenderer', 'style', 'tnode']);
const isPendingDelete = !!props.tnode.attributes.deleted;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Boolean(props.tnode.attributes.deleted); might be a little clearer. You could also do this inline on line 26 instead of making a separate variable for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const isWeb = [CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP].includes(getPlatform());

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: extra blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

return (
<Text
// eslint-disable-next-line react/jsx-props-no-spreading
{...defaultRendererProps}
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={isPendingDelete && isWeb ? [styles.offlineFeedback.deleted, styles.offlineFeedback.edited] : {}}
>
{/* Native devices do not support margin between nested text */}
<Text style={styles.w1}>{' '}</Text>
Expand Down
5 changes: 4 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 @@ -18,6 +18,7 @@ import * as StyleUtils from '../../../styles/StyleUtils';
import {withNetwork} from '../../../components/OnyxProvider';
import CONST from '../../../CONST';
import applyStrikethrough from '../../../components/HTMLEngineProvider/applyStrikethrough';
import getPlatform from '../../../libs/getPlatform';

const propTypes = {
/** The message fragment needing to be displayed */
Expand Down Expand Up @@ -107,11 +108,12 @@ const ReportActionItemFragment = (props) => {
// we render it as text, not as html.
// This is done to render emojis with line breaks between them as text.
const differByLineBreaksOnly = Str.replaceAll(html, '<br />', '\n') === text;
const isWeb = [CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP].includes(getPlatform());
Copy link
Contributor

Choose a reason for hiding this comment

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

@pubudu-ranasinghe Why is this necessary? We discourage the use of platform dependent code unless it is absolutely necessary.

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 issue doesn't happen on native apps. And providing inline-block to RN styles apparently throws an error. So needed to contain it only to desktop and web.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the error? If it is not supported by react-native, we better look for other solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It says it should be one of the supported values flex or none. Will look into more solutions.


// Only render HTML if we have html in the fragment
if (!differByLineBreaksOnly) {
const isPendingDelete = props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && props.network.isOffline;
const editedTag = props.fragment.isEdited ? '<edited></edited>' : '';
const editedTag = props.fragment.isEdited ? `<edited ${isPendingDelete ? 'deleted="true"' : ''}></edited>` : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Do we need ="true" here or can we just include deleted as boolean attribute?

Copy link
Contributor Author

@pubudu-ranasinghe pubudu-ranasinghe May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I've updated to use just the attribute key

const htmlContent = applyStrikethrough(html + editedTag, isPendingDelete);

return (
Expand All @@ -133,6 +135,7 @@ const ReportActionItemFragment = (props) => {
<Text
fontSize={variables.fontSizeSmall}
color={themeColors.textSupporting}
style={[...props.style, isWeb ? styles.offlineFeedback.edited : {}]}
>
{` ${props.translate('reportActionCompose.edited')}`}
</Text>
Expand Down
3 changes: 3 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2629,6 +2629,9 @@ const styles = {
textDecorationLine: 'line-through',
textDecorationStyle: 'solid',
},
edited: {
display: 'inline-block',
},
pending: {
opacity: 0.5,
},
Expand Down