-
Notifications
You must be signed in to change notification settings - Fork 11
Add wp-show
directive attribute
#156
Add wp-show
directive attribute
#156
Conversation
There was a problem hiding this 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!! 🙏
Sure, I'll do that 🙂 |
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. |
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. |
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). |
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.