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.

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

Correct hx-verb leaving behind empty class attributes on elements #2683

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

duanemoody
Copy link

Description

Manually added check for empty class to doSettle() after it removes its own classes:
elt.classList.length === 0 && elt.removeAttribute('class')

Corresponding issue:

#1701 reporting empty class attributes left behind after hx-verb actions. Other functions in htmx.js do classList.length checks but doSettle doesn't leverage any of them, so per the HTML spec empty class attributes are left behind.

Testing

I ran the modified code on elements with and without existing classes, and it behaved as expected: if existing class attributes were present with values, hx-put left them as-is; if no class attribute was present, none was left behind.

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

Telroshan and others added 7 commits June 27, 2024 13:36
Add
JavaScript Jabber htmx 2.0
Spiro Floropoulos
Software Unscripted
ThePrimeTime
TKYT
Add `elt.classList.length === 0 && elt.removeAttribute('class')` to remove empty class attributes left behind
Add `elt.classList.length === 0 && elt.removeAttribute('class')` to remove empty class attributes left behind
Add `elt.classList.length === 0 && elt.removeAttribute('class')` to remove empty class attributes left behind
Add `elt.classList.length === 0 && elt.removeAttribute('class')` to remove empty class attributes left behind
Add `elt.classList.length === 0 && elt.removeAttribute('class')` to remove empty class attributes left behind
@duanemoody
Copy link
Author

Technically I could have just rewritten the line in doSettle that removes the utility class via elt.classList.remove to instead use htmx's own removeClassFromElement method (which does housekeeping on blank class attributes) but I suspected there was a reason why doSettle doesn't use it.

@Telroshan
Copy link
Collaborator

Hey,

  • It may have been an oversight at the time it was written, by someone who maybe was simply not aware of removeClassFromElement's existence. Looking at the code, I can't think of a reason why it shouldn't be used here
  • I see there are other places when classList.remove or classList.add are called and they're likely to oversights just as much. Would you like to fix those at the same time ?
  • As per the contribution guidelines, could you please remove the dist files from the PR ?

Please avoid sending the dist files along your PR, only include the src ones.

All PRs must be made against the dev branch, except documentation PRs (that only modify the www/ directory) which can be made against master.

@duanemoody
Copy link
Author

It may have been an oversight at the time it was written, by someone who maybe was simply not aware of removeClassFromElement's existence. Looking at the code, I can't think of a reason why it shouldn't be used here

I see there are other places when classList.remove or classList.add are called and they're likely to oversights just as much. Would you like to fix those at the same time ?

I should target htmx.js only and not any of the other files by the sound of it. I'll get on normalizing the code.

Please avoid sending the dist files along your PR, only include the src ones.

Ouch, went overboard there.

As per the contribution guidelines, could you please retarget your PR to dev ?

Can do as soon as I figure out how to change that in GH.

@duanemoody duanemoody changed the base branch from master to dev June 28, 2024 16:22
Changes should not have been made in dist/
Changes should not be made in dist/
Changes should not be made in dist/
Changes should not be made in dist/
Replaced all element.classList add/remove methods with addClassToElement/RemoveClassFromElement
Replace all direct `classList` add/remove calls with htmx's own `addClassToElement`/`removeClassFromElement` methods, ensuring housecleaning of empty `class` elements left behind by remove.
Copy link
Author

@duanemoody duanemoody left a comment

Choose a reason for hiding this comment

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

Updated commit per requests to

  • Normalize replacement across the codebase
  • Target dev instead of master
  • Remove changes to files in dist/

@duanemoody
Copy link
Author

I commented after two lines that the original code used ic.classList.add/remove.call without a clear understanding of why that was necessary, but I suspect addClassToElement / removeClassFromElement already get enough context without needing call.

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

Successfully merging this pull request may close these issues.

None yet

3 participants