search

verbb / comments

A Craft CMS plugin for managing comments directly within the CMS.
Other
137 stars 33 forks source link

Adding some code to use/modify IDs if cloned divs have them #236

Closed adrienne closed 2 years ago

adrienne commented 2 years ago

When you clone the comment form (for editing or replying) you leave the existing ID, which can lead to odd issues if other JS is also running on the page. I refactored the ID generating code a little bit and applied a new ID to items created with .cloneNode(). It will use the existing ID as a prefix (that way if people have existing code that depends on an ID selector, they only need to modify it slightly to use a "starts with" selector like [id^=myprefix].

adrienne commented 2 years ago

@engram-design This isn't related to a specific issue but I had an issue where I had some other code running on textareas and it got confused when the ID was cloned. This is a simple solution to that problem, and also refactors the unique ID code that you were already using in one place to be more unique :)

engram-design commented 2 years ago

Thanks for putting this together!

Just to clarify, I can't see any id attribute on the comment form, which is the <article> element which wraps the form. Are you using custom templates and adding an ID to it?

If anything, I might I suggest simplifying the makeUniqueID() function a little? I'm just very careful with the overall size of the JS as it's a front-end resource. It doesn't need to be incredibly unique and to the length you define (although it does produce a nice and unique string).

adrienne commented 2 years ago

@engram-design - so this works on an individual comment form, and only if it starts out with an ID. (Your default form.twig template has no ID but our overridden version does.)

I can tweak the ID code but i really do think it needs a stronger uniqueness guarantee than you seem to think it does! (We have a LOT of comments on some of these entries.) And it's only a few lines long.

engram-design commented 2 years ago

Gotcha, I thought it might be your unique scenario with overrides. I just wanted to check I wasn't going mad not seeing something.

If that's the case, I don't see it messing with any existing (default, out-of-the-box) behaviour, and for that reason, happy to merge.

adrienne commented 2 years ago

@engram-design Yeah, i specifically set it up so that if there isn't an ID, it doesn't add one (because if there isn't one, it's unlikely we care); it's just that if there IS an ID, the old cloning behavior wasn't taking that into account, so we ended up with multiple forms on the page with the same ID. This was a problem because we also had third-party JS running that was doing a character counter on the form, and it was getting weirdly passed between forms and just generally being a mess 😄

Thanks for merging this!