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

Add wp-show directive attribute #156

Merged

Conversation

SantosGuillamot
Copy link
Contributor

This pull request aims to replicate the behavior of the wp-show directive tag (component), but using a directive attribute instead.

It's true that in terms of functionality, the directive tag should be enough for now, but both approaches should be available. Especially if we end up removing directive tags (components) from the proposal.

In the movies demo, we will use this directive, so it would be great if we could just use directive tags.

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.

The code looks fine to me, but would you mind adding some e2e tests? Thanks!! 🙏

@SantosGuillamot
Copy link
Contributor Author

The code looks fine to me, but would you mind adding some e2e tests? Thanks!! 🙏

Sure, I'll do that 🙂

@SantosGuillamot
Copy link
Contributor Author

I started adding some tests, but something is not working as expected. It seems the HTML generated is not correct, and I don't know why it seems to be passing the attributes to the next HTML tag. I'll review tomorrow in more detail why that is happening.

@SantosGuillamot
Copy link
Contributor Author

I've added more tests to check the toggling and that it works with context as well -> Commit.

In this other commit, @luisherranz fixed the issue I mention here. However, we have to decide What should be the HTML structure returned when wp-show is false? before merging this PR. The current approach is working, but if we go with the other alternative, the problem mentioned still persists.

@luisherranz
Copy link
Member

luisherranz commented Feb 22, 2023

Let's merge this as it is and if we decide to go with the opposite approach, we'll change it.

I'll investigate the bug a bit and open another issue, as it's not related to this PR (it's a general bug).

@SantosGuillamot SantosGuillamot merged commit 77aed62 into main-wp-directives-plugin Feb 23, 2023
@SantosGuillamot SantosGuillamot deleted the add/wp-show-directive-attribute branch February 23, 2023 10:51
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.

None yet

2 participants