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

RichText 🧹 (clean up, move Autocomplete, remove DOM dependency) #16905

Merged
merged 8 commits into from
Aug 14, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Aug 5, 2019

I renamed this branch because @frontdevde had problems pulling Gutenberg because of the emoji in the branch name.

Description

This PR mostly moves code around.

  • Move Autocomplete to the RichText wrapper with block context.
  • Properly convert the multiline prop to a multilineTag.
  • Use the rich text value to determine when to merge. This removes the dependency on the DOM package.
  • Split onKeyDown in multiple functions.
  • Reduce code indentation (in one occasion 5 → 2 tabs).

How has this been tested?

E2e tests should pass.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality [Package] Rich text /packages/rich-text labels Aug 5, 2019
@ellatrix ellatrix added this to the Gutenberg 6.3 milestone Aug 5, 2019
@ellatrix ellatrix mentioned this pull request Aug 5, 2019
5 tasks
@ellatrix ellatrix changed the title RichText 🧹 RichText 🧹 (use hooks, split list logic, remove DOM dependency) Aug 7, 2019
@ellatrix ellatrix mentioned this pull request Aug 7, 2019
5 tasks
@ellatrix ellatrix force-pushed the try/rich-text-cleanup branch 3 times, most recently from 1f16b2f to 34cdf95 Compare August 7, 2019 18:28
@ellatrix ellatrix changed the title RichText 🧹 (use hooks, split list logic, remove DOM dependency) RichText 🧹 (clean up, move Autocomplete, remove DOM dependency) Aug 8, 2019
@gziolo gziolo removed this from the Gutenberg 6.3 milestone Aug 12, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This PR still needs to be rebased after some changes applied to master.

I tested a few blocks including Paragraph, Galler and List toghether with the autocompletion and everything works as expected.

I'm not fully skilled to process all changes around new onKeyDown implementation in the rich-text/src/component/index.js so you might want to ask someone else to focus on that part.

@ellatrix
Copy link
Member Author

I'm not fully skilled to process all changes around new onKeyDown implementation in the rich-text/src/component/index.js so you might want to ask someone else to focus on that part.-

I'm not sure who else would be more skilled. I merely moved some code around and sorted it by keyCode. The only change I made that is not just moving code around is swapping ! isHorizontalEdge, which uses the DOM, with

activeFormats.length ||
( isReverse && start !== 0 ) ||
( ! isReverse && end !== text.length )

so that we use the internal rich text value instead of the DOM to determine when to merge/delete.

It's the same thing.

Since we're at the start of a release cycle, I'll merge this now, so we can continue work on top of this, and there is enough time to catch problems if any appear.

@ellatrix ellatrix merged commit 24caaa3 into master Aug 14, 2019
@ellatrix ellatrix deleted the try/rich-text-cleanup branch August 15, 2019 11:17
@ellatrix ellatrix added this to the Gutenberg 6.4 milestone Aug 15, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* RichText 🧹

* Clean up

* Clean up

* Simplify

* Move Autocomplete

* children => Editable wrapper component

* Bind Editable wrapper to class

* Minimise diff
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* RichText 🧹

* Clean up

* Clean up

* Simplify

* Move Autocomplete

* children => Editable wrapper component

* Bind Editable wrapper to class

* Minimise diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants