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

Simplify quote values #5265

Closed
wants to merge 2 commits into from
Closed

Simplify quote values #5265

wants to merge 2 commits into from

Conversation

ellatrix
Copy link
Member

Description

See #5252.

Currently there are isEqual checks on a React objects every time a quote block re-renders. This branch avoids these isEqual checks in the RichText component by changing the quote values to not get recalculated every time render is called.

How Has This Been Tested?

  • Make sure quotes render and transformations work.
  • Make sure you can move the caret to both fields in the quote.

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts labels Feb 26, 2018
@ellatrix
Copy link
Member Author

@youknowriad @aduth Not sure if the structure of the value attributes had a reason or if it was just so to work with hpq.

@ellatrix
Copy link
Member Author

Also makes me wonder if we should/could warn block authors if they do something similar (return new arrays/objects on every render).

{ value && value.map( ( paragraph, i ) =>
<p key={ i }>{ paragraph.children && paragraph.children.props.children }</p>
) }
{ [ value ] /* Prevent keys warning... */ }
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if we should do something else here... :)

@youknowriad
Copy link
Contributor

Nice exploration :), I like these improvements.

Not sure if the structure of the value attributes had a reason or if it was just so to work with hpq.

The structure was not necessary, it was there because the idea was that you can use any submatcher to your query matchers. In this PR, you forced the node sub matcher as default submatcher for query, I'm fine with that personally.

@aduth
Copy link
Member

aduth commented Feb 26, 2018

In this PR, you forced the node sub matcher as default submatcher for query, I'm fine with that personally.

I'm not as fine with the change. Notably the fact that it breaks the gallery block which leverages the generalized behavior of query.

@aduth
Copy link
Member

aduth commented Feb 26, 2018

Over the weekend I started on a branch to explore eliminating children and node, which may offer similar simplifications and otherwise conflict with this proposal.

@ellatrix
Copy link
Member Author

@aduth So what would you propose instead? I don't really care that much how the array of elements is created, just that it's not this weird structure with [ { children: {} } ].

@youknowriad
Copy link
Contributor

I'm not as fine with the change. Notably the fact that it breaks the gallery block which leverages the generalized behavior of query.

Not sure I understand, the generalized behavior is still here, this PR introduces a new behavior in the query source if you omit the query attribute. Another option would be a matcher: { type: 'node' } to make it generic.

If you have query you create an object if you don't have query you use the matcher. I believe we already had this discussion before @aduth :) (I wanted to introduce an object source at that time :)

@youknowriad
Copy link
Contributor

youknowriad commented Feb 26, 2018

related discussion in #2854

@aduth
Copy link
Member

aduth commented Feb 26, 2018

Actually, now that we're talking about it, I think the best simplification would be to upgrade the blocks to use block nesting.

@ellatrix
Copy link
Member Author

ellatrix commented Mar 6, 2018

Okay, how do me move this forward? Make it nested? Is this a valuable intermediate step or shall I close this PR. What I'd like is to get rid of the isEqual check though which is there only for the quote black because it's calculating new values all the time.

@ellatrix
Copy link
Member Author

ellatrix commented Mar 6, 2018

I added a message for block authors so they don't do the same... E.g. replace a RichText value prop with [ ...value ] and you'll see the warning:

screen shot 2018-03-06 at 23 44 52

@aduth
Copy link
Member

aduth commented Mar 6, 2018

I'm not a fan of the relationship between query and node that's being introduced here. My opinion is to abandon it and move forward with nested work.

@ellatrix ellatrix mentioned this pull request Apr 6, 2018
3 tasks
@ellatrix
Copy link
Member Author

ellatrix commented Apr 6, 2018

Continuing in #6054.

@ellatrix ellatrix closed this Apr 6, 2018
@ellatrix ellatrix deleted the try/better-quote-query branch April 6, 2018 20:50
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 [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants