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

feat: Migrate input bar to Lexical #15507

Merged
merged 97 commits into from
Oct 9, 2023
Merged

feat: Migrate input bar to Lexical #15507

merged 97 commits into from
Oct 9, 2023

Conversation

przemvs
Copy link
Contributor

@przemvs przemvs commented Jul 26, 2023

Description

This is a major refactor of the InputBar to now use https://lexical.dev/
This will fix issues like dealing with international keyboard better and also improve the overall architecture of the input bar

Note for the reviewers

This is a huge PR, no doubt about this. Note that:

  • most of the logic is hidden inside of the RichTextEditor folder, there lives the core of the input bar logic.

E2E test to run:

  • mentioning someone (with the @ symbol)
  • deleting a mention
  • reply to a message
  • switching conversation and check that text is still there (with mentions)
  • add emojis using the : symbol
  • inline replacement of emojis (eg. ;) should be converted to 😉 )
  • typing indicator (start typing, stop typing, delete entire message, send message, switch conversation)
  • markdown editing

To Fix:

  • searching for a user in the current conversation switches focus back to the inputbar
  • pressing arrow up when in mention mode will load the last sent message in edit mode

@przemvs przemvs marked this pull request as ready for review July 27, 2023 05:38
@przemvs przemvs requested review from a team and otto-the-bot as code owners July 27, 2023 05:38
@przemvs przemvs changed the title New input bar feat: New input bar Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #15507 (f04b2f7) into dev (cbe34d9) will decrease coverage by 0.39%.
The diff coverage is 31.25%.

@@            Coverage Diff             @@
##              dev   #15507      +/-   ##
==========================================
- Coverage   44.67%   44.28%   -0.39%     
==========================================
  Files         668      683      +15     
  Lines       22671    22920     +249     
  Branches     5169     5214      +45     
==========================================
+ Hits        10128    10150      +22     
- Misses      11245    11469     +224     
- Partials     1298     1301       +3     

let handled = false;
const nodeToSelect = getPreviousSibling(node);
if ($isElementNode(nodeToSelect)) {
nodeToSelect.selectEnd();
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you using only if blocks? using else if will make it more readable. Alternatively you can use separate helper functions for each type of node selection to make the code more self-explanatory.

 const selectEnd = ($isElementNode(node: Node)): void => {
    node.selectEnd();
    handled = true;
  };

  const selectText = ($isTextNode(node: Node)): void => {
    node.select();
    handled = true;
  };
  
   if ($isElementNode(nodeToSelect)) {
    selectEnd(nodeToSelect);
  } else if ($isTextNode(nodeToSelect)) {
    selectText(nodeToSelect);
  } 
  // rest of the code

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 comes from Mention plugin from Lexical, and this if's is more readable ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, it was a large block of if's without any description. its definitely not more readable.

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's change to one function named onArrowPress. So now we can reuse it for keys: ArrowLeft and ArrowRight and other keys if we want. So I'll add for You comment for what element we handling.

const nodeToSelect = getNextSibling(node);
if ($isElementNode(nodeToSelect)) {
nodeToSelect.selectStart();
handled = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

same feedback goes here too

@@ -1,6 +1,6 @@
/*
* Wire
* Copyright (C) 2022 Wire Swiss GmbH
* Copyright (C) 2023 Wire Swiss GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is an existing file then copyright should't change

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's not used, I remove hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is an existing file, for existing file we shouldn't change the copyright year.

src/script/components/LexicalInput/hooks/useIsFocused.ts Outdated Show resolved Hide resolved
@atomrc atomrc force-pushed the feature/WPB-460-new-input-bar branch from 7d67402 to beaf068 Compare September 4, 2023 12:29
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewers: please bear in mind that this plugin was copy/pasted from the lexical repo (and modified to work in reverse order)

@atomrc atomrc force-pushed the feature/WPB-460-new-input-bar branch from 8ca864f to 7a4d20e Compare September 4, 2023 15:06
};
}, [updateTheme]);
setTheme(getTheme());
useUserPropertyChange(getTheme, WebAppEvents.PROPERTIES.UPDATE.INTERFACE.THEME, setTheme);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since I needed the same logic for the "replaceEmoji" user preference, I extracted that logic to a more generic hook

export function HistoryPlugin(): null {
const [editor] = useLexicalComposerContext();

useEffect(() => registerHistory(editor, createEmptyHistoryState(), 300), [editor]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please move 300 to const, now we don't know what it mean :(

package.json Outdated
Comment on lines 6 to 7
"@lexical/history": "^0.11.3",
"@lexical/react": "^0.11.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to use exact versions here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

good call 👌

@atomrc atomrc merged commit 00c57a3 into dev Oct 9, 2023
11 checks passed
@atomrc atomrc deleted the feature/WPB-460-new-input-bar branch October 9, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants