-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
TwigComponent & LiveComponent Packages #101
Conversation
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
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.
Here are some quick things
|
||
```twig | ||
<!-- Add class after 200ms of loading --> | ||
<div data-loading="delay|addClass(opacity-50)">...</div> |
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 assume delay|addClass(opacity-50)
is parsed by js later.
* | ||
* @experimental | ||
*/ | ||
final class BeforeReRender |
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.
final class BeforeReRender | |
#[\Attribute(\Attribute::TARGET_METHOD)] | |
final class BeforeReRender |
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.
Good catch! The LAST little bit that we didn't quite finish was for the PHP 8 attributes. That will come shortly (even quite possibly today). In addition to this change, there are some changes to the code that parses through the annotations/attributes :)
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.
Ooh indeed, there is of course more to do than adding some attributes :D
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.
Here are some more comments.
Just nit picking. I have not been able to properly review logic etc.
src/LiveComponent/src/DependencyInjection/LiveComponentExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/EventListener/LiveComponentSubscriber.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydrator/DoctrineEntityPropertyHydrator.php
Outdated
Show resolved
Hide resolved
I'm not a UX expert and don't want to block this
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.
small comments
4db7e33
to
cf9cae5
Compare
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.
some reviews passing by :)
``` | ||
|
||
```twig | ||
<div {{ init_live_component(this) }}> |
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.
maybe add a twig file path?
Let's get started! Install the library with: | ||
|
||
``` | ||
composer require symfony/ux-live-component |
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.
what about symfony/ux-twig/component
in reference of the sentence Live components work with the TwigComponent
?
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.
Hmm, I don't quite understand. Can you say a bit more?
Thanks for the review :)
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.
@weaverryan sorry missing the word adding
:s
=> what about adding symfony/ux-twig/component
so that the line became composer require symfony/ux-live-component symfony/ux-twig/component
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.
Ah! It's just not needed - since symfony/ux-twig-component
is a dependency: so you'll get it automatically.
{ | ||
// TODO: Template attribute/annotation/interface to customize | ||
// TODO: Self-Rendering components? | ||
$templateName = sprintf('components/%s.html.twig', $component::getComponentName()); |
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.
maybe let this configurable? or call it twig_components? components
is a generic name
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.
We may actually move the entire template name into the component itself - e.g. via a getTemplate(): string
method. It seems like people are liking that better.
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.
it would also make it compatible with bundles providing some components, btw
} | ||
}, | ||
"require": { | ||
"php": ">=7.2.5", |
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 question, why the other is 7.4 and this ine 7.2?
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.
Excellent question! We originally targeted 7.2, but it became a burden on the other one. A final decision needs to be made - and these should probably be consistent.
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.
v7.4 would be top 👍
v7.2 (and v7.3) are eol or soon to be :)
exports.buildFormData = buildFormData; | ||
exports.buildSearchParams = buildSearchParams; | ||
|
||
function _typeof(obj) { "@babel/helpers - typeof"; if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { _typeof = function _typeof(obj) { return typeof obj; }; } else { _typeof = function _typeof(obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj; }; } return _typeof(obj); } |
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.
to avoid duplicating babel helpers in the app bundles, it might be better to configure the build process to import the babel helpers instead of copying them (and to add a dependency on @babel/helpers
of course)
* @property {string[]} args An array of unnamed arguments passed to the action | ||
* @property {Object} named An object of named arguments | ||
* @property {DirectiveModifier[]} modifiers Any modifiers applied to the action | ||
* @property {function} getString() |
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.
would be better to describe the signature of the function
@@ -0,0 +1,80 @@ | |||
"use strict"; | |||
|
|||
Object.defineProperty(exports, "__esModule", { |
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.
As our target are webpack users, it would be better to ship ES modules rather than CJS ones. Webpack has several optimizations that apply only on ES modules (module concatenation for instance, which helps a lot with minification)
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.
This is definitely one of the things that needs to be done before release - I was thinking we should use bundler to properly put this together. Thanks for mentioning this :)
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.
side note: should we even transpile those files for IE11 compat ? It increases the size for all projects even if they don't need IE11 support. It might be better to let projects needing IE11 to include those packages in the transpiled ones in the webpack config.
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 was actually thinking about exactly this too. Either not transpiling for IE11 and either document that users should include a polyfill package we provide (that's why I have the starting polyfill as a separate file right now) or just polyfill themselves. There is some work to be done here to optimize but make IE11 support clear
*/ | ||
private $fieldName = null; | ||
|
||
public function __construct(array $values) |
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.
To support attributes, I suggest you use separate arguments there, and you use doctrine annotation's @NamedArgumentConstructor
annotation to tell Doctrine about it
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.
note that to use @NamedArgumentConstructor
reliably, you should probably declare a conflict with doctrine/annotations <1.12
**EXPERIMENTAL** This component is currently experimental and is | ||
likely to change, or even change drastically. | ||
|
||
Live components work with the [TwigComponent](../TwigComponent) |
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.
this relative linki won't work when rendering the readme in subtree splits (and so not on packagist either)
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.
Of course - good catch! I'll fix after we have the splits - I'm keeping a list :)
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.
LGTM, especially since it's experimental :)
|
||
When you trigger an action, a POST request is sent that contains | ||
a `X-CSRF-TOKEN` header. This header is automatically populated | ||
and violated. In other words... you get CSRF protection without |
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 assume this is a typo 😀
and violated. In other words... you get CSRF protection without | |
and validated. In other words... you get CSRF protection without |
``` | ||
|
||
Ok: time to build that `post_form` component! The Live Components package | ||
come with a special trait - `ComponentWithFormTrait` - to make it easy to |
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.
come with a special trait - `ComponentWithFormTrait` - to make it easy to | |
comes with a special trait - `ComponentWithFormTrait` - to make it easy to |
Be sure to add the `@Assert\IsValid` to any property where you want | ||
the object on that property to also be validated. | ||
|
||
Tahnks to this setup, the component will now be automatically validated |
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.
Tahnks to this setup, the component will now be automatically validated | |
Thanks to this setup, the component will now be automatically validated |
Introducing 2 new packages: TwigComponent and LiveComponent