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

[bug] load trigger stops hx-disabled-elt getting re-enabled #2925

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

MichaelWest22
Copy link
Contributor

@MichaelWest22 MichaelWest22 commented Sep 23, 2024

Description

When we init nodes and find a "load" trigger it does the request straight away and this request may disable some elements with hx-disabled-elt. However if these disabled elements have not yet had their nodeInit() run yet because they are placed lower in the page then when they later init they first de-init their internalData which removes the requestCount counter used to re-enable them again. So we need it to handle situations where requestCount is not set and currently it defaults this to -1 and no 0 because of the decrement. So to solve this I've just changed it to fall back to 1 which means that if either requestCount is undefined or if it is equal to 0 it will replace it with 1 and then after the decrement will be 0 and trigger the proper code to enable the element again.

Corresponding issue:
#2767

Testing

Wrote a test to reproduce the issue with disabled-elt and load together and did a quick test in my test project to prove the disabled button re-enables as expected.

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

@MichaelWest22 MichaelWest22 changed the title Disable on load [bug] load trigger stops hx-disabled-elt getting re-enabled Sep 23, 2024
Copy link
Collaborator

@Telroshan Telroshan left a comment

Choose a reason for hiding this comment

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

This counter decrement was added in c1bf3fa
I won't speak for @1cg but I would guess that this was probably an overlook/copy-paste at the time ? The || 0 works well for the addition as we take it from 0 to 1, which makes sense, just serves as a "if undefined then 0"
But for the decrement, I don't see any reason why we'd want to reach a negative value for the counter, we'd instead want to reach 0 just like you did in this PR.

So, with the little I know (I wasn't there when it was first written), I would totally approve this change

@Telroshan Telroshan added bug Something isn't working ready for review Issues that are ready to be considered for merging labels Sep 23, 2024
@1cg 1cg merged commit 3d1a2e5 into bigskysoftware:dev Sep 25, 2024
1 check passed
@1cg
Copy link
Contributor

1cg commented Sep 25, 2024

Yep, just me being stupid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants