Interactivity API: Added comments for directive parsing#61456
Interactivity API: Added comments for directive parsing#61456amitraj2203 wants to merge 5 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
cbravobernal
added
[Type] Code Quality
|
@cbravobernal I've made the changes you suggested! Now, instead of using 'prefix' and 'suffix', I've renamed the variables more clearly. Thanks! |
cbravobernal
left a comment
cbravobernal
left a comment
There was a problem hiding this comment.
I'm leaving this on Request changes to wait for #61409 to be merged and also to get some more feedback about the naming of those vars.
packages/interactivity/src/vdom.ts
Outdated
| const directiveMatch = directiveParser.exec( name ); | ||
| // The reducer function accumulates the __directives object. | ||
| ( obj, [ directiveName, namespace, inputValue ] ) => { | ||
| // Check if the directive name matches the expected format. |
There was a problem hiding this comment.
| // Check if the directive name matches the expected format. |
Same here.
packages/interactivity/src/vdom.ts
Outdated
| props.__directives = directives.reduce( | ||
| ( obj, [ name, ns, value ] ) => { | ||
| const directiveMatch = directiveParser.exec( name ); | ||
| // The reducer function accumulates the __directives object. |
There was a problem hiding this comment.
| // The reducer function accumulates the __directives object. |
Same here.
packages/interactivity/src/vdom.ts
Outdated
| ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( `Invalid directive: ${ name }.` ); | ||
| console.warn( |
There was a problem hiding this comment.
Let's wait for #61409 to be merged and update with its function.
packages/interactivity/src/vdom.ts
Outdated
| const directiveType = directiveMatch[ 1 ] || ''; | ||
| const directiveInput = directiveMatch[ 2 ] || 'default'; | ||
|
|
||
| // Creating or updating the array for the specific directive type in the directives object. |
There was a problem hiding this comment.
| // Creating or updating the array for the specific directive type in the directives object. |
Same here
packages/interactivity/src/vdom.ts
Outdated
| namespace: ns ?? currentNamespace(), | ||
| value, | ||
| suffix, | ||
| // Splitting the directive name into directive type and input. |
There was a problem hiding this comment.
| // Splitting the directive name into directive type and input. |
Same here
packages/interactivity/src/vdom.ts
Outdated
| value, | ||
| suffix, | ||
| // Splitting the directive name into directive type and input. | ||
| const directiveType = directiveMatch[ 1 ] || ''; |
There was a problem hiding this comment.
| const directiveType = directiveMatch[ 1 ] || ''; | |
| const directive = directiveMatch[ 1 ] || ''; |
I think directive is a better name.
packages/interactivity/src/vdom.ts
Outdated
| suffix, | ||
| // Splitting the directive name into directive type and input. | ||
| const directiveType = directiveMatch[ 1 ] || ''; | ||
| const directiveInput = directiveMatch[ 2 ] || 'default'; |
There was a problem hiding this comment.
Could be "event" or "directiveEvent" a better name for this?
Not sure, cause they are events in data-wp-on-document or data-wp-on-window or data-wp-on. But can also be styles or classes in data-wp-class-- or data-wp--style. Even attributes for data-wp-bind
What do you think about it? Let's ping some folks with experience developing with the Interactivity API to get some feedback.
Ping: @ryanwelcher , @luisherranz , @DAreRodz or @sethrubenstein
|
Hi @cbravobernal I have made changes as per your suggestion. Now waiting for replies from other folks. |
|
Sorry @amitraj2203 but we are not going to merge this PR. The prefix word is being used through all the Interactivity codebase, and is a bigger refactor than expected. |
2 participants
What?
Fixes: #61455
Why?
It adds comments for directive parsing
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast