Skip to content

[Form] Update form_collections.rst #15981

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

Merged
merged 1 commit into from
Jan 14, 2022
Merged

[Form] Update form_collections.rst #15981

merged 1 commit into from
Jan 14, 2022

Conversation

smbpunt
Copy link
Contributor

@smbpunt smbpunt commented Oct 21, 2021

Hi,

I followed the instruction, and if you add a delete button to the <ul> parent, you have a delete button with no li to delete.

Thanks

@carsonbot carsonbot added this to the 5.4 milestone Oct 21, 2021
@javiereguiluz javiereguiluz requested a review from xabbuh November 18, 2021 16:48
@carsonbot carsonbot changed the title Update form_collections.rst [Form] Update form_collections.rst Nov 18, 2021
@@ -531,7 +531,7 @@ First, add a "delete this tag" link to each tag form:

.. code-block:: javascript

const tags = document.querySelectorAll('ul.tags')
const tags = document.querySelectorAll('ul.tags.li')
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 really be ul.tags.li or ul.tags li ?

Hi,

Shouldn't we add a delete link just to <li> childs instead of the <ul> parent ?

Thanks
@wouterj wouterj changed the base branch from 5.4 to 5.3 January 14, 2022 21:49
@wouterj wouterj merged commit bce26da into symfony:5.3 Jan 14, 2022
@wouterj
Copy link
Member

wouterj commented Jan 14, 2022

Hi @Punt13140! Looking at the old jQuery version of this example (https://symfony.com/doc/4.4/form/form_collections#template-modifications), Javier is correct and it should have been ul.tags li. I've applied his suggestion to your PR and merged it.

Thank you for fixing bugs in the documentation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants