Skip to content
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

Use getAttribute to find duplicate ids, not the id property #880

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Use getAttribute to find duplicate ids, not the id property #880

merged 1 commit into from
Dec 13, 2024

Conversation

ethancrawford
Copy link
Contributor

Since form controls can be accessed in javascript directly by name through the form object, (e.g. my_form.my_input), c.id may end up referring to an element whose name is id, instead of referring to the global id attribute. Explicitly retrieving the id with getAttribute avoids this.

Fixes #879

@ethancrawford
Copy link
Contributor Author

Happy to adjust if requested 👍

@sgelbart
Copy link

sgelbart commented Feb 24, 2023 •
edited
Loading

disregard approval! accident... (thanks for all your hardwork on this though!)

@ethancrawford
Copy link
Contributor Author

Ha! No worries! 👍

@ethancrawford ethancrawford force-pushed the fix-append-duplicate-forms branch from be3ff38 to 3bfd3a6 Compare February 27, 2023 03:07
@ethancrawford
Copy link
Contributor Author

Incidentally, if anyone is able to discuss with me (or point to somewhere I can ask appropriately) about how I can make @hotwired/turbo-rails in one of my projects use my patched version of turbo until this fix (or at least another like it that fixes the issue it is addressing) is officially available, I would be very grateful. (I have attempted to pull this fix into a project I'm working on with a "resolutions" field added to package.json for yarn, pointing towards the Git commit of my fix, but I'm unable to get yarn build to succeed because the turbo-rails source code, which says things like import * as Turbo from "@hotwired/turbo" causes yarn to say "Could not resolve "@hotwired/turbo". Can anyone help, or where should I ask? 🙂

@seanpdoyle
Copy link
Contributor

This feels related to #1037.

seanpdoyle
seanpdoyle approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@seanpdoyle seanpdoyle left a comment

Choose a reason for hiding this comment

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

@afcapel @kevinmcconnell could you enable CI to run for this PR?

@ethancrawford
Copy link
Contributor Author

Are folks happy enough with a force push here for me to correct the conflicts, or is a new commit preferred?

@afcapel
Copy link
Collaborator

afcapel commented Oct 23, 2023

@seanpdoyle I don't think I can run the CI due to the merge conflicts.

@ethancrawford force push is ok. Re. using a custom version of Turbo I think your best option is build the branch yourself and vendor the resulting dist file in your app. You can remove the vendored file once there's a release with your changes.

@ethancrawford
Copy link
Contributor Author

Sure, thanks!

@ethancrawford ethancrawford force-pushed the fix-append-duplicate-forms branch from 3bfd3a6 to 8c94c7c Compare October 23, 2023 13:03
@brunoprietog
Copy link
Collaborator

@ethancrawford can you do rebase? Hopefully this can be merged soon

@ethancrawford
Use getAttribute to find duplicate ids, not the id property
f0928ab 
Since form controls can be accessed in javascript directly by name through the form
object, (e.g. `my_form.my_input`), `c.id` may end up referring to an _element_
whose name is `id`, instead of referring to the global id _attribute_.
Explicitly retrieving the id with `getAttribute` avoids this.
@ethancrawford ethancrawford force-pushed the fix-append-duplicate-forms branch from 8c94c7c to f0928ab Compare December 9, 2023 05:25
@ethancrawford
Copy link
Contributor Author

@brunoprietog done - apologies for not noticing the latest conflict sooner!

@brunoprietog brunoprietog mentioned this pull request Dec 9, 2023
Open
brunoprietog
brunoprietog approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

Thanks @ethancrawford!

@brunoprietog brunoprietog merged commit 1007de9 into hotwired:main Dec 13, 2024
1 check passed
@ethancrawford
Copy link
Contributor Author

You're welcome! 👍

@ethancrawford ethancrawford deleted the fix-append-duplicate-forms branch December 13, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoprietog brunoprietog brunoprietog approved these changes

@sgelbart sgelbart sgelbart approved these changes

@seanpdoyle seanpdoyle seanpdoyle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stream append/prepend can produce duplicates if the template element is a form
5 participants
@ethancrawford @sgelbart @seanpdoyle @afcapel @brunoprietog