Skip to content

[LiveComponent] Tweak live collection rendering #421

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 14, 2022
Merged

[LiveComponent] Tweak live collection rendering #421

merged 1 commit into from
Aug 14, 2022

Conversation

1ed
Copy link
Contributor

@1ed 1ed commented Aug 11, 2022 •
edited
Loading

Q A
Bug fix? yes
New feature? yes?
Tickets -
License MIT

@1ed 1ed changed the title tweak live collection rendering [LiveComponent] Tweak live collection rendering Aug 11, 2022
@1ed 1ed force-pushed the tweak-live-collection-rendering branch from ad02c31 to a8c60b7 Compare August 11, 2022 19:16
weaverryan
weaverryan reviewed Aug 11, 2022
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot! Would you mind adding a small section to the docs with an example of overriding live_collection_widget (e.g. to render the add button first then the widget) and live_collection_entry_row. (e.g. to add a <div> around each row)?

@1ed 1ed mentioned this pull request Aug 11, 2022
Merged
@1ed
Copy link
Contributor Author

1ed commented Aug 12, 2022

I like this a lot! Would you mind adding a small section to the docs with an example of overriding live_collection_widget (e.g. to render the add button first then the widget) and live_collection_entry_row. (e.g. to add a <div> around each row)?

Thanks! I will.

1ed
1ed commented Aug 12, 2022
View reviewed changes
src/LiveComponent/src/Resources/views/form_theme.html.twig Outdated
'data-action-name': 'addCollectionItem(name=' ~ form.vars.full_name ~ ')'
}) }) }}
{%- if skip_add_button|default(false) is same as false and button_add_prototype is defined and not button_add_prototype.rendered -%}
{{ form_row(button_add_prototype) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about changing the name of the exposed variable to button_add or maybe something more specific to the collection live_collection_item_add or just item_add?

skip_add_button, button_add, button_delete?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_button, delete_button?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I kind of like button_add and button_delete better, but not sure about it 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that.

1ed
1ed commented Aug 13, 2022
View reviewed changes
1ed
1ed commented Aug 13, 2022
View reviewed changes
weaverryan
weaverryan reviewed Aug 14, 2022
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I've left some questions about the docs - my form theming is a bit fuzzy :)

src/LiveComponent/src/Resources/doc/index.rst Outdated

If you want more control over how each row is rendered you can override the blocks
related to the ``LiveCollectionType``. This works the same way as for `the traditional
collection type`_, but you should use ``live_collection`` and ``live_collection_entry`` as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean live_collection_widget here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I just meant to mention it's just a prefix, it can be live_collection_widget, live_collection_row, live_collection_errors, ... and linked the docs for the collection field (https://symfony.com/doc/current/form/form_themes.html#fragment-naming-for-collections). Maybe better to write live_collection_* instead.

src/LiveComponent/src/Resources/doc/index.rst Outdated

.. code-block:: twig

{%- block _blog_post_form_comments_widget -%}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this live_collection_widget? I mean, I know what you have is correct for this situation, but I think showing live_collection_widget would be better (it would be very rare someone would need to override LiveCollectionType twice in the same form in a different way).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant this could be an example for customizing just for a specific form type, and the next with the bootstrap table could be a generic one. But it's fine by me to keep it simple and just use live_collection_widget here too.

src/LiveComponent/src/Resources/doc/index.rst
<tr>
{% for child in form|filter(child => not child.rendered) %}
<td>{{- form_row(child, { label: false }) -}}</td>
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a clever way to loop over all of the fields in the sub-form and render them? If so, what's the purpose of the |filter(child => not child.rendered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this just renders the children in a <td>. It's safer to use in this way, because if a field was rendered already, rendering it again will throw an exception. But in this case it's not really necessary, so I leave it out.

@1ed 1ed force-pushed the tweak-live-collection-rendering branch 6 times, most recently from aa68ee3 to 0fa9688 Compare August 14, 2022 19:45
@weaverryan weaverryan force-pushed the tweak-live-collection-rendering branch from 0fa9688 to e174a28 Compare August 14, 2022 23:14
@weaverryan
Copy link
Member

Thank you Gábor!

@weaverryan weaverryan merged commit 569acef into symfony:2.x Aug 14, 2022
@weaverryan weaverryan mentioned this pull request Aug 14, 2022
Merged
weaverryan added a commit that referenced this pull request Aug 14, 2022
@weaverryan
minor #430 [LiveComponent] Minor docs tweaks and changelog bump (weav…
73c6569 
…erryan)

This PR was merged into the 2.x branch.

Discussion
----------

[LiveComponent] Minor docs tweaks and changelog bump

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Tickets       | Minor tweaks after #421
| License       | MIT

Commits
-------

a405475 [LiveComponent] Minor docs tweaks and changelog bump
@1ed
Copy link
Contributor Author

1ed commented Aug 14, 2022

Thank you!

@1ed 1ed deleted the tweak-live-collection-rendering branch August 14, 2022 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@kbond kbond kbond left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@1ed @weaverryan @kbond