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
Merged

Remove wp-show and wp-text directives #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1a79d0f
Remove text directive and its tests
DAreRodz Apr 18, 2023
f455cc7
Remove show directive and its tests
DAreRodz Apr 18, 2023
aaffee3
Replace wp-show with wp-bind.hidden in e2e tests
DAreRodz Apr 18, 2023
79bce9e
Temporarily skip test for duplicated hydration
DAreRodz Apr 18, 2023
56c756e
Merge branch 'refactoring-for-gutenberg' into remove-show-and-text-di…
luisherranz Apr 21, 2023
6f617d2
Add todo for skipped test
luisherranz Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions e2e/html/directives-show.html
View file
Open in desktop

This file was deleted.

37 changes: 0 additions & 37 deletions e2e/html/directives-text.html
View file
Open in desktop

This file was deleted.

23 changes: 11 additions & 12 deletions e2e/html/tovdom-islands.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

<span data-testid="not inside an island">
This should be shown because it is inside an island.
</span>
</div>

<div data-wp-island>
<div data-wp-show="state.falseValue">
<div data-wp-bind.hidden="!state.falseValue">
<span data-testid="inside an island">
This should not be shown because it is inside an island.
</span>
Expand All @@ -21,7 +21,7 @@

<div data-wp-island>
<div data-wp-ignore>
<div data-wp-show="state.falseValue">
<div data-wp-bind.hidden="!state.falseValue">
<span
data-testid="inside an inner block of an isolated island"
>
Expand All @@ -34,22 +34,21 @@

<div data-wp-island>
<div data-wp-island>
<div
data-wp-show="state.falseValue"
data-testid="island inside another island"
>
<span>
This should not have two template wrappers because
that means we hydrated twice.
</span>
<div data-testid="island inside another island">
<div data-wp-bind.hidden="!state.falseValue">
<span>
This should not have two template wrappers
because that means we hydrated twice.
</span>
</div>
</div>
</div>
</div>

<div data-wp-island>
<div>
<div data-wp-island data-wp-ignore>
<div data-wp-show="state.falseValue">
<div data-wp-bind.hidden="!state.falseValue">
<span
data-testid="island inside inner block of isolated island"
>
Expand Down
48 changes: 0 additions & 48 deletions e2e/specs/directives-show.spec.ts
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions e2e/specs/directives-text.spec.ts
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion e2e/specs/tovdom-islands.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ test.describe('toVdom - isands', () => {
await expect(el).toBeVisible();
});

test('directives inside islands should not be hydrated twice', async ({
// TODO: Implement this test once we have data-wp-init:
// https://github.com/WordPress/block-interactivity-experiments/pull/220#discussion_r1171417552
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.

page,
}) => {
const el = page.getByTestId('island inside another island');
Expand Down
35 changes: 0 additions & 35 deletions src/runtime/directives.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,25 +129,6 @@ export default () => {
}
);

// data-wp-show
directive(
'show',
({
directives: {
show: { default: show },
},
element,
evaluate,
context,
}) => {
const contextValue = useContext(context);
if (!evaluate(show, { context: contextValue }))
element.props.children = (
<template>{element.props.children}</template>
);
}
);

// data-wp-ignore
directive(
'ignore',
Expand All @@ -164,20 +145,4 @@ export default () => {
);
}
);

// data-wp-text
directive(
'text',
({
directives: {
text: { default: text },
},
element,
evaluate,
context,
}) => {
const contextValue = useContext(context);
element.props.children = evaluate(text, { context: contextValue });
}
);
};