Handle context menu format bold and italic #556

bernhardoj · 2024-12-04T15:58:03Z

Details

Handle the context menu Format command for Bold and Italic.

Related Issues

Expensify/App#53145

Manual Tests

Run the web example app on Safari web/mWeb
Type anything on the text input
Select any of the text
Right-click/long-press and choose Format context menu option
Choose Bold or Italic
Verify it wraps the selected text with the selected markdown (* for Bold, _ for Italic)

ios.mweb.mp4

web.mp4

Linked PRs

tomekzaw · 2024-12-04T16:16:00Z

Hi @bernhardoj, thanks for submitting this PR. I'm okay with the changes.

However, this PR only implements Format menu on web. I believe the same feature is missing on native iOS. Are there any plans to implement this missing feature also in native iOS?

@Skalakid and @BartoszGrajdek Could you please review and test this PR? Thanks in advance.

bernhardoj · 2024-12-05T05:44:39Z

I don't see the Format option on iOS native. I think it's only available on the web.

ios.2.mp4

tomekzaw · 2024-12-05T09:13:19Z

@bernhardoj You're right, if there's no menu on native iOS then we don't need to add one right now.

In that case I think we can proceed with web-only implementation.

tomekzaw · 2024-12-05T09:17:59Z

src/MarkdownTextInput.web.tsx

+          default:
+            markdown = '';


I think this state is unreachable so we could just throw an error here.

Is it better if I add underline as the default value?

switch (formatCommand) { case 'formatBold': formatType = 'bold'; break; case 'formatItalic': formatType = 'italic'; break; default: formatType = 'underline'; break; }

Skalakid · 2024-12-05T15:21:17Z

Hello @bernhardoj, thank you for your PR!

Yesterday, we merged support for adding custom parsers to the Live Markdown Input - PR. It's a pretty big change, and it extends the use cases for our library. Because of that, I think we should slightly update your solution in this PR. Now you've implemented adding static syntaxes in the format function, like: * for bold and _ for italics. However, because now users can use other custom parsers together with the Live Markdown library, it would be perfect to allow them to choose other custom syntaxes for their styles. They can implement for example a parser that transforms following /sample text/ string into bold and use / instead of *.
To solve this problem, how about adding the formatSelection prop to the Live Markdown Input, so users can handle this case on their own? In the case of Expensify, it will look something like this:

formatSelection={(selectedText: string, style: 'bold' | 'italic' | 'underline') => style === 'bold' ? `*${selectedText}*` : style === 'italic' ? `_${selectedText}_` : selectedText}

What do you think about it?

bernhardoj · 2024-12-05T16:40:45Z

That sounds good, I'll update it tomorrow!

bernhardoj · 2024-12-06T15:13:29Z

@Skalakid I need help with defining the type for formatSelection. Since this is only for web, is it fine to add the type to MarkdownTextInputProps on both MarkdownTextInput.tsx and MarkdownTextInput.web.tsx files?

react-native-live-markdown/src/MarkdownTextInput.tsx

Lines 54 to 57 in 9ad0acd

    
           interface MarkdownTextInputProps extends TextInputProps, InlineImagesInputProps { 
        
             markdownStyle?: PartialMarkdownStyle; 
        
             parser: (value: string) => MarkdownRange[]; 
        
           }

react-native-live-markdown/src/MarkdownTextInput.web.tsx

Lines 30 to 36 in 9ad0acd

    
           interface MarkdownTextInputProps extends TextInputProps, InlineImagesInputProps { 
        
             markdownStyle?: MarkdownStyle; 
        
             parser: (text: string) => MarkdownRange[]; 
        
             onClick?: (e: MouseEvent<HTMLDivElement>) => void; 
        
             dir?: string; 
        
             disabled?: boolean; 
        
           }

formatSelection?: (selectedText: string, formatType: FormatType) => string;

tomekzaw · 2024-12-07T21:03:32Z

@bernhardoj Just FYI, I was able to add "Format" button in context menu on native iOS with the following code snippet:

// inside UITextView
UIMenuItem *formatItem = [[UIMenuItem alloc] initWithTitle:@"Format" action:@selector(command:textView:)];
UIMenuController.sharedMenuController.menuItems = @[formatItem];

- (void)command:(nonnull UICommand *)command textView:(nonnull UITextView *)textView
{
  // Do something
}

bernhardoj · 2024-12-09T06:35:27Z

Should I add it here?

react-native-live-markdown/apple/RCTUITextView+Markdown.mm

Lines 29 to 40 in 5695408

    
           + (void)load 
        
           { 
        
             static dispatch_once_t onceToken; 
        
             dispatch_once(&onceToken, ^{ 
        
               Class cls = [self class]; 
        
               SEL originalSelector = @selector(textDidChange); 
        
               SEL swizzledSelector = @selector(markdown_textDidChange); 
        
               Method originalMethod = class_getInstanceMethod(cls, originalSelector); 
        
               Method swizzledMethod = class_getInstanceMethod(cls, swizzledSelector); 
        
               method_exchangeImplementations(originalMethod, swizzledMethod); 
        
             }); 
        
           }

Also, I get this error when trying to run the iOS example app.

error export CLANG_WARN_DOCUMENTATION_COMMENTS=YES
error export CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER=NO
error export GCC_WARN_UNDECLARED_SELECTOR=YES
error export VALIDATE_PRODUCT=NO
error Failed to build ios project. "xcodebuild" exited with error code '65'. To debug build logs further, consider building your app with Xcode.app, by opening 'LiveMarkdownExample.xcworkspace'.
npm ERR! Lifecycle script ios failed with error:
npm ERR! Error: command failed
npm ERR! in workspace: @expensify/[email protected]
npm ERR! at location: /Users/bernhardoj/Documents/dev/freelance/react-native-live-markdown/example

tomekzaw · 2024-12-09T06:37:12Z

@bernhardoj I think we can implement native iOS in a separate PR. Once we merge #520 it should be a bit simpler to implement that. I think we'll need to send a RN event from the native side to the JS side when bold or italic button in context menu is pressed so the function can modify the React state of the component in order to modify the current value by adding/removing some characters when toggling bold/italic.

tomekzaw · 2024-12-09T07:22:09Z

@bernhardoj Also, I'm curious what's the expected behavior when *foo* is selected and "Bold" button is pressed. I would assume that formatting should be removed then, i.e. the asterisks should be removed.

bernhardoj · 2024-12-09T10:09:25Z

I would assume that formatting should be removed then, i.e. the asterisks should be removed.

I tested using the contenteditable and it indeed removes the bold when we select the bold format again. Also, Safari nicely shows that the Bold format is selected (I noticed that it shows the Bold as selected as long as the selected text has a bold font-weight style).

I wonder how we are going to handle the "unwrap" since we now allow a custom parser and handle cases like only selecting part of the bolded text (*b|ol|d*) or a combination of bolded and non-bolded texts (*bo|ld* nor|mal).

Also, the header markdown shows Bold format as selected.

Skalakid · 2024-12-09T10:48:31Z

Since this is only for web, is it fine to add the type to MarkdownTextInputProps on both MarkdownTextInput.tsx and MarkdownTextInput.web.tsx files?

@bernhardoj I think, yes. You can add it to both files. Thanks to that there won't be any TS errors in the app

bernhardoj · 2024-12-10T03:26:05Z

@Skalakid updated.

Also, I'm curious what's the expected behavior when foo is selected and "Bold" button is pressed. I would assume that formatting should be removed then, i.e. the asterisks should be removed.

I want to see how WhatsApp handles this case, but WA doesn't handle the format command on the web at all.

src/MarkdownTextInput.web.tsx

bernhardoj · 2024-12-17T02:31:28Z

@Skalakid hi, how's the review going?

Skalakid · 2024-12-17T13:21:31Z

Hi, I reviewed the PR, and the code looks good to me, however, together with @tomekzaw, we noticed that this issue is more complex than we thought. The current implementation allows to modify the selected text, however, there are many other cases connected to removing style from the text, that should be handled. Here is a list of them:

Removing style when all the text between style syntaxes is selected:
*[world]* -> [world]
In this case the "*" syntaxes from outside the text selection, should be removed. In current implementation we are only returning the currently selected text so it will be hard to remove them
Removing style from the middle of the text:
*hello [world] test* -> *hello* [world] *test*
The same situation here, with by only having the selected text (world) it would be hard to split the styled text in the middle

I think we should reconsider the current solution and create simple to use API for react-native-live-markdown library that will allow to implement style formatting in both directions (adding and removing the style). How about adding more props to the formatSelection function like: selection start, selection end, active markdown ranges in the current selection. What's your thought on this? Do you have any other solution for that?

Skalakid · 2024-12-17T13:42:51Z

On the other hand, I checked now, and WhatsApp doesn't support removing the styles in the format option.

ScreenRecording_12-17-2024.14-13-21_1.MP4

So I think we can merge it as it is and enhance the formatSelection function in the future

Skalakid · 2024-12-17T13:45:27Z

src/MarkdownTextInput.web.tsx

+          default:
+            formatType = 'underline';
+            break;
+        }


Why underline is set as default?

I actually asked about this here. Since we only handle the bold, italic, and underline command, I default it to underline. Should I use the "throw" approach?

Skalakid · 2024-12-17T13:48:06Z

src/MarkdownTextInput.web.tsx

+        let formatType: FormatType;
+        switch (formatCommand) {
+          case 'formatBold':
+            formatType = 'bold';
+            break;
+          case 'formatItalic':
+            formatType = 'italic';
+            break;
+          default:
+            formatType = 'underline';
+            break;
+        }


Let's move this switch elsewhere, for example to blockUtils.ts

Skalakid · 2024-12-17T13:48:55Z

src/MarkdownTextInput.web.tsx

+            break;
+        }
+
+        const beforeSelectedText = parsedText.slice(0, contentSelection.current.start);


Suggested change

const beforeSelectedText = parsedText.slice(0, contentSelection.current.start);

const prefix = parsedText.slice(0, contentSelection.current.start);

Skalakid · 2024-12-17T13:49:46Z

src/MarkdownTextInput.web.tsx

+        const selectedText = parsedText.slice(contentSelection.current.start, contentSelection.current.end);
+        const formattedText = formatSelection?.(selectedText, formatType) ?? selectedText;
+        const formattedTextDiffLength = formattedText.length - selectedText.length;
+        const afterSelectedText = parsedText.slice(contentSelection.current.end);


Suggested change

const afterSelectedText = parsedText.slice(contentSelection.current.end);

const suffix = parsedText.slice(contentSelection.current.end);

Skalakid · 2024-12-17T13:50:27Z

src/MarkdownTextInput.web.tsx

+        const beforeSelectedText = parsedText.slice(0, contentSelection.current.start);
+        const selectedText = parsedText.slice(contentSelection.current.start, contentSelection.current.end);
+        const formattedText = formatSelection?.(selectedText, formatType) ?? selectedText;
+        const formattedTextDiffLength = formattedText.length - selectedText.length;


Suggested change

const formattedTextDiffLength = formattedText.length - selectedText.length;

const diffLength = formattedText.length - selectedText.length;

Skalakid · 2024-12-17T13:52:58Z

src/MarkdownTextInput.web.tsx

+
+        const beforeSelectedText = parsedText.slice(0, contentSelection.current.start);
+        const selectedText = parsedText.slice(contentSelection.current.start, contentSelection.current.end);
+        const formattedText = formatSelection?.(selectedText, formatType) ?? selectedText;


We can check if the returned text actually changed after running formatSelection, to avoid unnecessary text parsing

Skalakid · 2024-12-17T13:56:23Z

src/MarkdownTextInput.web.tsx

+        if (!contentSelection.current) {
+          return {
+            text: '',
+            cursorPosition: 0,
+          };
+        }


How about early returning also when contentSelection length between start and end is less than 1?

blimpich · 2024-12-23T17:19:04Z

@Skalakid @tomekzaw friendly bump on re-reviewing this PR 🙂

mananjadhav · 2025-01-01T11:19:26Z

@Skalakid @tomekzaw quick bump on this one.

bernhardoj mentioned this pull request Dec 4, 2024

[$250] mWeb - Compose box- Selected text isn't applied format Expensify/App#53145

Open

8 tasks

tomekzaw requested review from Skalakid and BartoszGrajdek December 4, 2024 16:07

tomekzaw reviewed Dec 5, 2024

View reviewed changes

feat: handle context menu format command

292f1d8

bernhardoj force-pushed the feat/handle-ctx-menu-format branch from c732cd4 to 292f1d8 Compare December 10, 2024 03:19

Skalakid requested changes Dec 11, 2024

View reviewed changes

src/MarkdownTextInput.web.tsx Outdated Show resolved Hide resolved

chore: rename format function

8f5716b

blimpich assigned bernhardoj Dec 16, 2024

Skalakid requested changes Dec 17, 2024

View reviewed changes

bernhardoj added 3 commits December 18, 2024 00:58

Early return when selection length is < 1

2dd64f6

Skip slicing text if the formatted text is unchanged

8bc3c47

chore: move the format command to type conversion to a util function

bce6f0a

blimpich requested review from tomekzaw and Skalakid January 2, 2025 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle context menu format bold and italic #556

Handle context menu format bold and italic #556

bernhardoj commented Dec 4, 2024

tomekzaw commented Dec 4, 2024 •

edited

Loading

bernhardoj commented Dec 5, 2024

tomekzaw commented Dec 5, 2024

tomekzaw Dec 5, 2024

bernhardoj Dec 6, 2024

Skalakid commented Dec 5, 2024 •

edited

Loading

bernhardoj commented Dec 5, 2024

bernhardoj commented Dec 6, 2024

tomekzaw commented Dec 7, 2024

bernhardoj commented Dec 9, 2024

tomekzaw commented Dec 9, 2024 •

edited

Loading

tomekzaw commented Dec 9, 2024

bernhardoj commented Dec 9, 2024

Skalakid commented Dec 9, 2024 •

edited

Loading

bernhardoj commented Dec 10, 2024

bernhardoj commented Dec 17, 2024

Skalakid commented Dec 17, 2024 •

edited

Loading

Skalakid commented Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

Skalakid Dec 17, 2024

bernhardoj Dec 17, 2024

blimpich commented Dec 23, 2024

mananjadhav commented Jan 1, 2025

	const beforeSelectedText = parsedText.slice(0, contentSelection.current.start);
	const prefix = parsedText.slice(0, contentSelection.current.start);

	const afterSelectedText = parsedText.slice(contentSelection.current.end);
	const suffix = parsedText.slice(contentSelection.current.end);

	const formattedTextDiffLength = formattedText.length - selectedText.length;
	const diffLength = formattedText.length - selectedText.length;

Handle context menu format bold and italic #556

Are you sure you want to change the base?

Handle context menu format bold and italic #556

Conversation

bernhardoj commented Dec 4, 2024

Details

Related Issues

Manual Tests

Linked PRs

tomekzaw commented Dec 4, 2024 • edited Loading

bernhardoj commented Dec 5, 2024

tomekzaw commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skalakid commented Dec 5, 2024 • edited Loading

bernhardoj commented Dec 5, 2024

bernhardoj commented Dec 6, 2024

tomekzaw commented Dec 7, 2024

bernhardoj commented Dec 9, 2024

tomekzaw commented Dec 9, 2024 • edited Loading

tomekzaw commented Dec 9, 2024

bernhardoj commented Dec 9, 2024

Skalakid commented Dec 9, 2024 • edited Loading

bernhardoj commented Dec 10, 2024

bernhardoj commented Dec 17, 2024

Skalakid commented Dec 17, 2024 • edited Loading

Skalakid commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimpich commented Dec 23, 2024

mananjadhav commented Jan 1, 2025

tomekzaw commented Dec 4, 2024 •

edited

Loading

Skalakid commented Dec 5, 2024 •

edited

Loading

tomekzaw commented Dec 9, 2024 •

edited

Loading

Skalakid commented Dec 9, 2024 •

edited

Loading

Skalakid commented Dec 17, 2024 •

edited

Loading