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

Remove React-related attributes from pullquote multi-paragraph block fixtures #7448

Closed
wants to merge 1 commit into from

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Jun 21, 2018

This is a suggestion to improve the multi-paragraph pullquote test fixtures.

Description

The tests for the multi-paragraph pullquote are storing React internal data in the fixtures. That's problematic because its values might change unexpectedly depending on the tests run before. Indeed, I had to deal with it in #6549 (see https://github.com/WordPress/gutenberg/pull/6549/files#r189686931).

To avoid it and make the test simpler we can remove the nested elements from the parsed content. Similar tests don't have nested elements either in the fixtures.

How has this been tested?

It's a change to the fixtures of a single block, so running npm test should be enough.

Types of changes

Test improvement.

Checklist:

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

…fixtures

Using nested elements in the pullquote multi-paragraph value was making the
parser to store React properties. That was producing unexpected effects when
other tests were added. In order to keep those tests with a single purpose
it's better to keep the values simple without nested elements, so it doesn't
depend on the React version in use.
@aduth
Copy link
Member

aduth commented Jun 21, 2018

Related: #771

In a brief discussion which took place at WordCamp Europe between myself and @youknowriad , an alternative here may be to simplify the object shape as the minimal possible value which our custom serializer is capable of supporting, namely an object with type and props (with children).

I worry the changes as proposed here merely sweep the problem under the rug rather than address the true issue.

@Aljullu
Copy link
Contributor Author

Aljullu commented Jun 23, 2018

@aduth you're right. In this PR I just wanted to fix one specific test that was failing in some circumstances.

#771 seems to have a much broader scope. I also saw #7476, which might make my PR no longer necessary.

So we might leave this PR on hold until we see what happens with the other proposed solutions.

@aduth
Copy link
Member

aduth commented Sep 13, 2018

This should no longer be relevant after #7934.

@aduth aduth closed this Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants