-
Notifications
You must be signed in to change notification settings - Fork 33
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
React 18 compatibility #2268
React 18 compatibility #2268
Conversation
Most of the libraries used by the modules today are shipping with WP, no need to rebundle them.
This is not a solution since no replacement is provided. The todo note is a reminder to come back to this.
…alendar/tribe-common into fix/TEC-5322-wp-67-packages
The php-date-formatter package does not really export anything, so it cannot be aliased in webpack externals. The next best thing is moving the script to Common and providing a handle for it.
While we need the plugin text domain in the matching function, no need to load markup and translate information.
406b23c
to
01ab890
Compare
…alendar/tribe-common into fix/TEC-5322-wp-67-packages
a4d4a23
to
b21c56a
Compare
cc8883c
to
b29019c
Compare
This updates the `data` attribute to use a less generic name and modifies the `click` handler logic to target IAN elements only. Additionally, ensure elements exist in the current context before targeting them.
89547e7
to
6fd025a
Compare
6fd025a
to
1142ac7
Compare
…alendar/tribe-common into fix/TEC-5322-wp-67-packages
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.
Just a file movement in order for our final zips to include php-date-formatter.js
.
Everything else looks great!
"node_modules/php-date-formatter/js/php-date-formatter.js", | ||
"node_modules/php-date-formatter/js/php-date-formatter.min.js", |
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.
Can we include those outside of node_modules ?
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.
I think the build system should support this, changing the way dependencies are managed, in node_modules
and through npm
, to go with something that is fixed (in vendor
or includes
) seems working around a bug.
The node_modules
directory is being included when ET is built.
Furthermore, this is both included directly, and imported in the blocks.
Could the build system be fixed?
@@ -258,6 +258,7 @@ public function load_assets() { | |||
[ 'tribe-attrchange', 'vendor/attrchange/js/attrchange.js' ], | |||
[ 'tec-ky-module', 'vendor/ky/ky.js', [], null, [ 'module' => true ] ], | |||
[ 'tec-ky', 'vendor/ky/tec-ky.js', [ 'tec-ky-module' ], null, [ 'module' => true ] ], | |||
[ 'tec-common-php-date-formatter', 'node_modules/php-date-formatter/js/php-date-formatter.js' ], |
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 file source would need to be replaced here as well
Ticket: TEC-5322
Update asset version, registration and usage to align with React 18.