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

SSR: Handle attribute directives separately from tag directives #140

Merged

Conversation

ockham
Copy link
Collaborator

@ockham ockham commented Jan 26, 2023

Follow-up to #133, addressing the following feedback:

I'm not sure if it'd be better to divide the definition of directive attributes and directive tags into different functions as we do in JavaScript. Something like process_wp_bind_tag and process_wp_bind_attribute.

(#133 (comment))

and

I wonder what would be more performant, doing get_attribute_names_with_prefix once per directive attribute, or just doing it once to get all the attributes and search for the directives ourselves.

(#133 (comment))

  • Create new subfolders attributes and tags inside both src/directives/ and phpunit/directives/.
  • In wp-directives.php, use separate arrays for attribute and tag directives.
  • Remove now-obsolete code and tests from attribute directive processors (that would've dealt with being called on a tag directive).
  • Differentiate process_wp_context_tag and process_wp_context_attribute.
  • Disable unit test for wp-context attribute directive, as it's not implemented yet.

@ockham ockham self-assigned this Jan 26, 2023
@ockham ockham marked this pull request as ready for review January 26, 2023 14:22
Copy link
Collaborator

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Everything OK codewise. Approved! ✅

By the way, I see that you started calling directives to both tags and attributes. The idea I had in that regard was that just the attributes were directives, while the tags were simply components (like in Web Components).

I mean, the names you used are OK. I guess we'll reach a consensus with the terminology anytime soon, haha. 😄

@ockham
Copy link
Collaborator Author

ockham commented Jan 31, 2023

Everything OK codewise. Approved! ✅

Thank you very much!

By the way, I see that you started calling directives to both tags and attributes. The idea I had in that regard was that just the attributes were directives, while the tags were simply components (like in Web Components).

Yeah, this is something that I believe I've discussed with @luisherranz (I thought it was at #118 but I can't find it right now) -- it seemed to me like Luis preferred that nomenclature over directive/component. I'm fine with either -- maybe "component" is a bit more likely to be confusing, since it's used in so many different contexts 🤔

I mean, the names you used are OK. I guess we'll reach a consensus with the terminology anytime soon, haha. 😄

Indeed! 😄

@ockham ockham merged commit bb6abaa into main-wp-directives-plugin Jan 31, 2023
@ockham ockham deleted the update/treat-attribute-directives-separately branch January 31, 2023 14:53
@luisherranz
Copy link
Member

luisherranz commented Feb 2, 2023

It was actually @dmsnell the first one who started using it. I adopted it right away because I thought it makes much more sense. I think others (@SantosGuillamot? @michalczaplinski?) have also been using it lately.

I guess we'll reach a consensus with the terminology anytime soon, haha.

You're totally right. We haven't discussed it anywhere. Sorry about that, David.

I'll add it to the Tracking Issue:

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.

3 participants