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.

Sign up for GitHub

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

Jump to bottom

Refactor onChangeEditableValue handling (version2) #12102

Closed
wants to merge 1 commit into from
Closed

Refactor onChangeEditableValue handling (version2) #12102

wants to merge 1 commit into from

Conversation

atimmer
Copy link
Member

@atimmer atimmer commented Nov 20, 2018 •
edited
Loading

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 alternative to #12099. This PR is cleaner, without duplication at the expense of performance.

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
Refactor onChangeEditableValue handling
6cfad12 
Move it to the `rich-text` package.
@atimmer atimmer force-pushed the try/refactor-onChangeEditableValueV2 branch from d68e9b7 to 6cfad12 Compare November 20, 2018 12:53
@atimmer atimmer requested a review from ellatrix November 20, 2018 12:53
@gziolo gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Jan 31, 2019
@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.

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

Fixed in #14284.

@ellatrix ellatrix closed this Mar 20, 2019
@ellatrix ellatrix deleted the try/refactor-onChangeEditableValueV2 branch March 20, 2019 16:34
@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
Reviewers

@ellatrix ellatrix Awaiting requested review from ellatrix

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@atimmer @gziolo @ellatrix @omarreiss