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

Refactor onChangeEditableValue handling (version1) #12099

Closed
wants to merge 1 commit into from

Conversation

atimmer
Copy link
Member

@atimmer atimmer commented Nov 20, 2018

Description

In #11861, @iseulde pointed out that ideally RichText should not 'know' anything about custom formats. So the handling of onChangeEditableValue should move to the rich-text package. Specifically the create function.

This is an attempt to execute this refactor. It works functionally, but the implementation has a ton of duplication. The problem I was trying to solve was that toFormat strips the relevant information we need when building the list of formats for the value. So to circumvent this, I build another array with shadowFormats that contains all formats that are actually in the DOM.

This shadowFormats array is then send to onChangeEditableValue.

Note: The actual onChangeEditableValue in RichText needs to move too, but I wanted to have these versions out asap to address the complexity and moving that function itself over is trivial so can be done after choosing how to refactor.

How has this been tested?

The e2e tests still pass, I have tested that annotations still work.

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.

@atimmer atimmer force-pushed the try/refactor-onChangeEditableValue branch from 9d2e44a to 5739ab9 Compare November 20, 2018 11:24
@atimmer atimmer changed the title Refactor onChangeEditableValue handling Refactor onChangeEditableValue handling (version1) Nov 20, 2018
Move it to the `rich-text` package.
@atimmer atimmer force-pushed the try/refactor-onChangeEditableValue branch from 5739ab9 to 16b4bd6 Compare November 20, 2018 12:51
@gziolo
Copy link
Member

gziolo commented Jan 31, 2019

@atimmer and @iseulde is this is something that is still necessary now that 5.0 was released? I'm adding Stale label to make it easier to triage open PRs. Feel free to remove if you plan to continue work on this PR.

@gziolo gziolo added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. labels Jan 31, 2019
@ellatrix
Copy link
Member

Yeah, I need to look into it when I find some time...

@atimmer atimmer mentioned this pull request Mar 6, 2019
5 tasks
@ellatrix
Copy link
Member

Fixed in #14284.

@ellatrix ellatrix closed this Mar 20, 2019
@ellatrix ellatrix deleted the try/refactor-onChangeEditableValue branch March 20, 2019 16:35
@omarreiss omarreiss added the [Feature] Annotations Adding annotation functionality label Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Annotations Adding annotation functionality [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants