Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Remove wp-show and wp-text directives #220

Merged
merged 6 commits into from
Apr 21, 2023

Conversation

DAreRodz
Copy link
Collaborator

This PR eliminates wp-show and wp-text directives, as they're not required for now. The wp-link directive, which is also not needed, has already been removed in #216.

@DAreRodz DAreRodz requested a review from luisherranz April 18, 2023 18:01
@@ -28,7 +28,7 @@ test.describe('toVdom - isands', () => {
await expect(el).toBeVisible();
});

test('directives inside islands should not be hydrated twice', async ({
test.skip('directives inside islands should not be hydrated twice', async ({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I skipped this one because I didn't see clearly how to refactor this test without using wp-show. @luisherranz, can you take a look at this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can remove this test for now, although I would prefer to have this case covered.

Copy link
Member

Choose a reason for hiding this comment

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

We can do something like this:

<div data-wp-island>
  <div data-wp-island>
    <div data-testid="island inside another island">
      <div data-wp-init="actions.appendText">
        <span>
          This should not have two spans appended because that means we hydrated
          twice.
        </span>
      </div>
    </div>
  </div>
</div>
appendText: ({ ref }) => {
  const span = document.createTextNode("Span");
  span.innerText = "some text";
  ref.appendChild(span);
};

And check that there's only one span or something similar.

But I think ref is currently broken due to this bug and data-wp-init is not implemented yet, so we can wait until then.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

LGTM, David!

@@ -28,7 +28,7 @@ test.describe('toVdom - isands', () => {
await expect(el).toBeVisible();
});

test('directives inside islands should not be hydrated twice', async ({
test.skip('directives inside islands should not be hydrated twice', async ({
Copy link
Member

Choose a reason for hiding this comment

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

We can do something like this:

<div data-wp-island>
  <div data-wp-island>
    <div data-testid="island inside another island">
      <div data-wp-init="actions.appendText">
        <span>
          This should not have two spans appended because that means we hydrated
          twice.
        </span>
      </div>
    </div>
  </div>
</div>
appendText: ({ ref }) => {
  const span = document.createTextNode("Span");
  span.innerText = "some text";
  ref.appendChild(span);
};

And check that there's only one span or something similar.

But I think ref is currently broken due to this bug and data-wp-init is not implemented yet, so we can wait until then.

@@ -5,14 +5,14 @@
</head>
<body>
<div>
<div data-wp-show="state.falseValue">
<div data-wp-bind.hidden="!state.falseValue">
Copy link
Member

@luisherranz luisherranz Apr 19, 2023

Choose a reason for hiding this comment

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

There's a state.trueValue as well if you prefer.

@DAreRodz DAreRodz merged commit 2260dd0 into refactoring-for-gutenberg Apr 21, 2023
@DAreRodz DAreRodz deleted the remove-show-and-text-directives branch April 21, 2023 09:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants