Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interactivity API: Improve readability in directives check function inside vdom.ts #61455

Closed
cbravobernal opened this issue May 7, 2024 · 3 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Packages] Interactivity /packages/interactivity [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality

Comments

@cbravobernal
Copy link
Contributor

          This section could probably use some comments about how strings are being split up and what we expect to find in each part. We've even got a constant `wp` that's the `directivePrefix` (that also appears in the middle of the data attribute).

Honestly, some kind of description of what the parts of a directive are and a refactor to rename things accordingly would be helpful. prefix and suffix don't really make sense:


nothing?       suffix is everything after `prefix--`… I guess this makes sense
   |              |
vvvvvvv       vvvvvvvvvv   
data-wp-bind--on--change
        ^^^^
          |
      "prefix" in the middle?

Maybe we could talk about these things like the "directive name" or "directive type", then the rest of it could be some kind of input to the directive? I think we use these prefix/suffix names across the package and it seems confusing.


The bug fix here where we try to access a null match is good 👍

Originally posted by @sirreal in #61249 (comment)

@cbravobernal cbravobernal added [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Code Quality Issues or PRs that relate to code quality labels May 7, 2024
@cbravobernal cbravobernal added Good First Issue An issue that's suitable for someone looking to contribute for the first time [Packages] Interactivity /packages/interactivity and removed [Feature] Interactivity API API to add frontend interactivity to blocks. labels May 7, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label May 7, 2024
@amitraj2203
Copy link
Contributor

amitraj2203 commented May 7, 2024

Hi @cbravobernal, I have added the general comment for this issue in this PR. Please let me know if we need to update it with some other content.

Thank you!

@cbravobernal
Copy link
Contributor Author

Hi @cbravobernal, I have added the general comment for this issue in this PR. Please let me know if we need to update it with some other content.

Thank you!

Thanks for contributing!

I left some feedback, but I would love more input for other folks.

Naming variables is a tough task! 😅

@cbravobernal
Copy link
Contributor Author

Finally I decided we are not going to go with this issue/PR.
The terms prefix and suffix appears through all Interactivity codebase , and is not something we can change slightly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Packages] Interactivity /packages/interactivity [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants