Page MenuHomePhabricator

[native] Add chat mention rule to text message rules
ClosedPublic

Authored by patryk on Aug 17 2023, 6:50 AM.
Tags
None
Referenced Files
F3140671: D8849.diff
Sun, Nov 3, 5:03 AM
Unknown Object (File)
Thu, Oct 31, 12:55 PM
Unknown Object (File)
Mon, Oct 28, 1:53 AM
Unknown Object (File)
Sun, Oct 27, 2:58 PM
Unknown Object (File)
Sun, Oct 27, 2:58 PM
Unknown Object (File)
Sun, Oct 27, 2:58 PM
Unknown Object (File)
Sun, Oct 27, 2:58 PM
Unknown Object (File)
Sun, Oct 27, 2:58 PM
Subscribers

Details

Summary

This diff adds chat mention rule to markdown rules file. Raw chat mentions should be replaced with MarkdownChatMention component.

Depends on D8874.

Test Plan

Please see test plans in diffs listed below:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

patryk held this revision as a draft.
patryk edited the test plan for this revision. (Show Details)
patryk added reviewers: tomek, inka, rohan.
patryk added inline comments.
native/chat/text-message-markdown-context.js
20 ↗(On Diff #30007)

See more info here.

native/markdown/rules.react.js
411–422 ↗(On Diff #30007)

Nit: shorthand

rohan requested changes to this revision.Aug 21 2023, 5:50 PM

Just a suggestion inline (not blocking the diff). Only thing blocking is the CI issue - not not sure what's going on with the CI failure, looks like it's a problem with useTextMessageRulesFunc only having one being passed one argument but requiring two

native/markdown/rules.react.js
417 ↗(On Diff #30007)

Since MarkdownChatMention is it's own component, you could probably handle the styling directly in there since it's just being bolded (will be easier in case the styles get more complicated down the line)

This revision now requires changes to proceed.Aug 21 2023, 5:50 PM

Probably it failed because of D8873 previous diff -> I forgot to add messageKey argument.

patryk edited the test plan for this revision. (Show Details)

Handle style condition in rules.react.js

Use SimpleMarkdown.inlineRegex

rohan requested changes to this revision.Aug 22 2023, 7:15 AM
rohan added inline comments.
native/markdown/rules.react.js
417 ↗(On Diff #30177)

Do we need to remove the style prop here now that it's been removed from the component in D8845

This revision now requires changes to proceed.Aug 22 2023, 7:15 AM

I think I misunderstood your feedback @rohan, did you mean that it's better to use styles inside a MarkdownChatMention component (use const styles = getMarkdownStyles(useDarkStyle ? 'dark' : 'light') inside the component and not passing styles as a prop)?

I think I misunderstood your feedback @rohan, did you mean that it's better to use styles inside a MarkdownChatMention component (use const styles = getMarkdownStyles(useDarkStyle ? 'dark' : 'light') inside the component and not passing styles as a prop)?

Oh I personally just felt like there's no need to pass in the actual style object since it looks like you're just using bold. My suggestion was to handle the style in the component with an unboundStyles object like in most native components. Something like:

function MarkdownChatMention(props: Props): React.Node {
  const { threadInfo, hasAccessToChat, ...rest } = props;
  const { shouldBePressable, onLongPressHandler } = useMarkdownOnPressUtils(
    !hasAccessToChat,
  );
  const styles = useStyles(unboundStyles);
  
  return (
    <Text
        onLongPress={shouldBePressable ? onLongPressHandler : null}
        style={styles.mention}
        {...rest}
      />
);

const unboundStyles = {
  mention: {
     fontWeight: 'bold',
  }
}

This would also make it easier if the styles for this component get more complicated down the line for some reason since they'd all be defined in an object in the component file. Though I see it's also a common pattern with some of the components in the file to just pass it in as a TextProp so I don't think it's too big of a deal and you could leave it as is

This revision is now accepted and ready to land.Aug 22 2023, 7:40 AM

Note that you don't need useStyles for something that doesn't use colors. In that case you could use StyleSheet.create directly

Render Text component directly in markdown rules to avoid a bug where
message "onPress area" was smaller when chat mention with no access was
rendered