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

allow hx-include/disabled-elt/indicator to be optional #2916

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

Conversation

MichaelWest22
Copy link
Contributor

@MichaelWest22 MichaelWest22 commented Sep 18, 2024

Description

This is a possible improvement to allow hx-include, hx-disabled-elt and hx-indicator to not report errors to the console when a selector finds no elements. Because of how some of these properties can inherit the use of find and similar extended selectors can fail to find any elements when triggered from a different element. When this happens it produces an error message to the console to warn you that it is not working as expected. I think this warning is still useful but it would be nice to have the option to disable the warning for situation where you expect this to happen.

So to allow this I've implemented an option to append a '?' to the end of the selector string which disables the error warning similar to how javascript and regex treat ? at the end as meaning optional.

So for example you can now use hx-include="input?" and if it can not find any inputs to include it will ignore this and not report any errors.

Corresponding issue:
#2895

Testing

Tests added and performed a little manual testing to test and debug the change in a test app

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
Copy link
Collaborator

Telroshan commented Sep 18, 2024

Hey, would be interested to have your opinion on #2902 !
I made changes to querySelectorAllExt directly in this one, which technically addresses the same issue since findAttributeTargets uses querySelectorAllExt (+ adds support for multiple extended selectors in hx-trigger's from clause), interested in your thoughts on that matter.

As for the optional change, I would indeed recommend making a separate PR for it, just to keep this down to 1 change to review to the final reviewer (that isn't me), will be easier to get each one approved separately than both at the same time I think!

@Telroshan Telroshan added the enhancement New feature or request label Sep 18, 2024
@MichaelWest22
Copy link
Contributor Author

Yeah Sorry I should have checked for that PR already being done!!! Your version of the fix is looking good and added a few comments there. Your fix handles one extra case I hadn't considered. So I will strip this PR back to just the optional change so there is less to review.

@MichaelWest22 MichaelWest22 changed the title fix multi selectors in hx-include/disabled-elt/indicator and add optional allow hx-include/disabled-elt/indicator to be optional Sep 18, 2024
www/content/attributes/hx-disabled-elt.md Outdated Show resolved Hide resolved
www/content/attributes/hx-indicator.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants