- Issue created by @fathershawn
- @fathershawn opened merge request.
- πΊπΈUnited States fathershawn New York
I have this working with in integration test. If anyone wants to see that locally, there is an
asset-tests
branch in the fork, which is this issue, plus the htmx dependency issue merged into the core 11.x branch. That allows the test to run. I'll bring that test over to this branch once the htmx dependency work is merged. - πΊπΈUnited States fathershawn New York
I hav two questions about merging drupalSettings.
ajax.js uses jQuery to merge the incoming drupalSettings value using the deep variation of
.extend()
if (response.merge) { $.extend(true, drupalSettings, response.settings); } else { ajax.settings = response.settings; }
In my contrib implementation I didn't want to start with a reliance on jQuery so adapted code from https://youmightnotneedjquery.com. I've kept this pattern in the MR at the moment, but I see that jQuery.extend() is used nearly a dozen other scripts. What's the best way forward? I see #3179551: Provide no-library equivalents of common/useful jQuery functions β but it doesn't include
.extend()
.2. The ajax API provides two options for drupalSettings above. I currently simply merge. What is the use case for not merging incoming settings?
I've added a Nightwatch test, and all Nightwatch tests are passing.
- π«π·France nod_ Lille
Thanks for the MR!
Glad to see this moving fast. Had a bunch of comment on the MR. Generally looks like things will work out :)
- πΊπΈUnited States fathershawn New York
Thinking about the feedback on an additional processor service I realized that we should be able to simplify that whole part of the solution by adding some appropriate conditions to the existing
HtmlResponseAttachmentsProcessor
. I'm hitting a wrinkle though which is thatHtmlResponseAttachmentsProcessor
gets called more than once preparing the response to my test page. This is an issue because if I callsetAlreadyLoadedLibraries
on the assets, then they stop getting added in subsequent runs. - πΊπΈUnited States fathershawn New York
fathershawn β changed the visibility of the branch asset-tests to hidden.
- @nod_ opened merge request.
- πΊπΈUnited States fathershawn New York
fathershawn β changed the visibility of the branch 3521173-process-attachments-cssjs to hidden.
- πΊπΈUnited States fathershawn New York
fathershawn β changed the visibility of the branch without-htmx-attachement-processor to hidden.
- π«π·France nod_ Lille
Simplified as much as I could, seems to work as expected.
- πΊπΈUnited States fathershawn New York
@nod_ requested two additional test cases in the prior PR.
- htmx powered elements inserted by our legacy ajax
- verify that our attach and detach behavior methods fire
I've added a route to the
test_htmx
module (/htmx-test-attachments/ajax) that creates markup for this test. In manual testing, htmx is inserted but does not process the inserted content. I think we need some glue code that calls htmx.process on markup inserted by our Ajax API.The only way I see to verify that methods are called in the Nightwatch test is to use the callTracker assertion but this is deprecated, so I'm wondering if someone has a better approach?
I also added some documentation links to the JS files.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- πΊπΈUnited States fathershawn New York
Nice simple solution @nod_. All tests green including the the added Ajax interaction test. What else do we need to move to RTBC?
- π³π±Netherlands bbrala Netherlands
Still open comments on the MR, those need adressing. :)
- πΊπΈUnited States fathershawn New York
I can't mark the MR comment resolved but I removed the version requirement from the info.yml file in the test module.
- πΊπΈUnited States fathershawn New York
All other unresolved MR comments are from the previous MR which we have closed.
- π¬π§United Kingdom longwave UK
Gonna go ahead and mark this RTBC so we can continue to move the HTMX proposal forward; given that this is currently experimental in core if we have to make any API changes or fixes then we should still be safe. Although it could do with another review from someone who writes more JavaScript than I do, I found the MR easy to follow and everything in it makes sense to me.
- π¬π§United Kingdom longwave UK
Also discussed this with @nod_ who pointed out that while we added the HTMX library already, it's not really much use without these changes - behaviors and assets are critical to integrating with the rest of Drupal's frontend - so tagging as an 11.2 release priority to allow others to start actually looking at how they might want to use this.