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.

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

Emojiundo #3405

Merged
merged 22 commits into from
Aug 14, 2020
Merged

Emojiundo #3405

merged 22 commits into from
Aug 14, 2020

Conversation

corinagum
Copy link
Contributor

TBD

@compulim compulim marked this pull request as ready for review August 14, 2020 10:27
@compulim
Copy link
Contributor

compulim commented Aug 14, 2020

Fixes #3249.

Changelog Entry

Added

Samples

Description

Added emoticon to Emoji in the send box.

Design

We designed this feature not to relies on setTimeout (like Teams), but event-based. This make the feature very robust when typing very quickly on the keyboard.

Style options

Enable Emojis (default)

{
  styleOptions: {
    emojiSet: true
  }
}

Disable Emojis

{ 
  styleOptions: { 
    emojiSet: false
  }
}

Or emojiSet: {}.

Custom Emoji set

{
  styleOptions: {
    emojiSet: {
      '100': '💯'
    }
  }
}

Hooks

It is not trivial for end-developer to re-build their own Emoji-enabled send box. This is because they must re-build their own undo stack. Otherwise, the "chemical reaction" between string.replace and React would mess up browser's undo stack.

Thus, we are not exposing any hooks. However, they can access the emojiSet style options through the existing useStyleOptions hook, for example:

const [{ emojiSet }] = useStyleOptions();

We are not exposing the useReplaceEmoticon.js because the API signature requires 3 things: value, selectionStart, and selectionEnd.

The selectionStart/selectionEnd seems not trivial for the user to understand their need. Thus, if they are rebuilding the send box with Emoji, they might not understand how to pass these arguments in and why they are needed.

FYI, they are needed because we need to know which character was changed. We are only replacing emoticons when the user finish the emoticon with its last character. I.e. user press D after the : (colon) letter, would turn it into :D and converted to 😁.

We are not replacing emoticons for the whole string as that would make the UI very difficult to use (every key press will convert to Emoji).

Thus, the useReplaceEmoticon hook is internal for now.

FYI, although if we used prevValue + value pair for API signature, it is simpler and potentially exposable. But it will not work and is no better than the selectionStart + value pair. This is because, given :) is in the box (as emoticon), appending ), or inserting ) between the letters, will turn it into :)). The first operation must not be converted. But the second operation must be converted.

But these operations are not distinguishable from each other when using prevValue + value. But it is distinguishable when using the selectionStart + value pair.

Clipboard or typing

We do not want to convert emoticons to Emojis when the user paste the text. Instead, the conversion should only be done when the user type on the send box.

Technically, we cannot distinguishable whether the user type a letter, or the user pasted a letter. We are using our best effort to guess if the user is pasting the text (≧ 2 characters) or typing the letter (= 1 character).

We also failed this case but IMO, this is marginal:

  1. Type :), it will turn into Emoji 😊
  2. Press CTRL + Z to undo back to :) (emoticon)
  3. Highlight the letter )
  4. Press )
  5. Expect it should be converted to Emoji 😊. But due to the limitation mentioned above, it is not converted

This is not perfect but this is as far as we could go.

Redo

We did not implement CTRL + Y (redo) yet.

Escaping

We did not implement escaping.

Specific Changes

  • Modified SendBox/TextBox.js
    • Re-built undo stack
    • Add emoji conversion
  • Added internal/useReplaceEmoticon.js
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
    • It is up to screen reader to read the Emoji
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
    • We are not bringing culture-specific emoticons (e.g. :wink:)
  • package.json and package-lock.json reviewed
  • Tests reviewed (coverage, legitimacy)

Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Looks good, although rather complex.

event => {
const { ctrlKey, key, metaKey } = event;

if ((ctrlKey || metaKey) && (key === 'Z' || key === 'z')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ctrl+Z typical for undo?

Copy link
Contributor

Choose a reason for hiding this comment

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

CTRL + Z is de facto on Windows. And I guess COMMAND + Z is de facto on Mac.

You spotted the corner I cut here. 😁

Please let me know if I should only allow COMMAND + Z on Mac but not CTRL + Z.

@compulim
Copy link
Contributor

Thanks @a-b-r-o-w-n.

Agree, rather complex.

OS will push undo checkpoint on:

  • Just before first change after focus
  • Just before first change after cursor move
  • Just before first change after cut/paste

But since HTML doesn't have cross-browser beforechange/changing event. So it looks complicated here. I need to keep remembering the current value just before the change event.

Better, I should "subclass" <input>/<textarea> and add a beforechange/changing event in the custom component. That could significantly simplify the code.

@compulim compulim merged commit 246ad4f into microsoft:master Aug 14, 2020
@compulim compulim deleted the emojiundo branch August 14, 2020 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants