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

Remove splitting ranges on emojis on the web #598

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+16 −7
Open

Remove splitting ranges on emojis on the web #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37e8ba0
Remove splitting ranges on emojis on the web
Skalakid Jan 15, 2025
451a308
Remove example content changes
Skalakid Jan 16, 2025
2fe0e40
Move sorting before splitting
Skalakid Jan 16, 2025
710c51a
Move comment
Skalakid Jan 16, 2025
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
15 changes: 9 additions & 6 deletions src/parseExpensiMark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,16 @@ function parseExpensiMark(markdown: string): MarkdownRange[] {
);
return [];
}
const sortedRanges = sortRanges(ranges);
let markdownRanges = sortedRanges;
Comment on lines +255 to +256
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const sortedRanges = sortRanges(ranges);
let markdownRanges = sortedRanges;
let markdownRanges = sortRanges(ranges);

if (Platform.OS === 'android') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since parseExpensiMark is a worklet, let's extract Platform.OS outside to avoid capturing whole Platform object in worklet closure.

const isAndroid = Platform.OS === 'android';

function parseExpensiMark(...) {
  if (isAndroid) { /* ... */ }
}

// Blocks applying italic and strikethrough styles to emojis on Android
// TODO: Remove this condition when splitting emojis inside the inline code block will be fixed on the web
markdownRanges = splitRangesOnEmojis(markdownRanges, 'italic');
Skalakid marked this conversation as resolved.
Show resolved Hide resolved
markdownRanges = splitRangesOnEmojis(markdownRanges, 'strikethrough');
}

let splittedRanges = splitRangesOnEmojis(ranges, 'italic');
splittedRanges = splitRangesOnEmojis(splittedRanges, 'strikethrough');

const sortedRanges = sortRanges(splittedRanges);
const groupedRanges = groupRanges(sortedRanges);

const groupedRanges = groupRanges(markdownRanges);
Copy link

Choose a reason for hiding this comment

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

Maybe do the sorting again here.

Copy link
Collaborator Author

@Skalakid Skalakid Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need it here. Sorting influences only the web and since we blocked spitting ranges for this platform, the output will still be correctly sorted

return groupedRanges;
}

Expand Down
8 changes: 7 additions & 1 deletion src/web/utils/blockUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@ function addStyleToBlock(targetElement: HTMLElement, type: NodeType, markdownSty
node.style.textDecoration = 'line-through';
break;
case 'emoji':
Object.assign(node.style, {...markdownStyle.emoji, verticalAlign: 'middle'});
Object.assign(node.style, {
...markdownStyle.emoji,
verticalAlign: 'middle',
fontStyle: 'normal',
textDecoration: 'none',
Comment on lines +32 to +33
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fontStyle: 'normal',
textDecoration: 'none',
fontStyle: 'normal', // remove italic
textDecoration: 'none', // remove strikethrough

display: 'inline-block',
});
break;
case 'mention-here':
Object.assign(node.style, markdownStyle.mentionHere);
Expand Down
Loading