-
-
Notifications
You must be signed in to change notification settings - Fork 394
[LiveComponent] Add a generic LiveCollectionType #324
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
Conversation
kbond
left a comment
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 great, thanks for working on this @1ed. Perfect use for live components!
| @@ -0,0 +1,35 @@ | |||
| {%- block live_collection_widget -%} | |||
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 this should be a different name. Maybe live_collection_form_theme.html.twig?
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 there should be only one theme per component (and per variant like e.g. bootstrap or tailwind) and not per type, and maybe later we would like to add more blocks for an another type, so I would keep the name. WDYT?
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 points, makes sense to me.
weaverryan
left a comment
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 this is a really cool idea! Let's go for it!
| 'data-action': 'live#action', | ||
| 'data-action-name': 'addCollectionItem(name=' ~ form.vars.full_name ~ ')' | ||
| }) }) }} | ||
| {% endif %} |
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's funny, #90 adds this at the top of the list, and you have it at the bottom. Nothing will work for everyone, but the bottom I think DOES seem 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.
Best would be probably both places :) saying this from experience when I've implemented 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.
Let's start with one... but it occurs to me that it may be be useful to configure this easily (e.g. top, bottom or both)
|
What is the difference with this PR? |
|
This requires your entire form be a live component, #90 does not. Basically, if your form already is a live component, just this would be required to add this functionality. |
weaverryan
left a comment
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 really wonderful work ❤️ . We may need more docs to mention that you should customize the live_collection_widget and live_collection_entry_row blocks in order to position the buttons just how you want them. I'm excited to try this in a real project and see how it goes :).
0c42b52 to
b1500fd
Compare
|
Thank you Gábor! |
|
Thank you Ryan! |
Hello, this is just a quick draft, extracted from a project, to see if is it interesting enough to include here.
TODO