nod_ → changed the visibility of the branch 3535173-support-dynamic-forms-only to hidden.
We should move the changes to the config form in a new issue like we have for 📌 Ajaxify the user interface translation forms Active so that we can use the new Htmx object so that we can start using the helper.
Just rebased the MR so that it'll work with the last version
Discussed on slack and I'm +1 for the simplification
Just a sidenote about the attribute class
/**
* {@inheritdoc}
*/
public function offsetSet($name, $value): void {
// If the value is not an AttributeValueBase object, then create one.
if (!($value instanceof AttributeValueBase)) {
$value = $this->createAttributeValue($name, $value);
}
$this->storage[$name] = $value;
}
A little problem with this is that $name could be different than what's $value has been created with. Not sure why it'd happen but it's very easy to run into.
In any case I'm happy with the MR so RTBC +1
major since it's an important api addition → (for DX)
porting credits from the other issues too
Committed and pushed 9640f56040d to 11.x and d798718197f to 11.2.x. Thanks!
@nicxvan not sure how we'd test this reliably. Maybe check if there are any AttributeValueBase in the #attributes array at the preprocess step? but then which render array do we test? or maybe a bunch of asserts?
@fathershawn: now that you're a maintainer you can give credit, credits are given for closed issues now too :)
let's call it to have someone take a look :)
nod_ → changed the visibility of the branch 3535173-support-dynamic-forms to hidden.
We've had a discussion on slack with @fathershawn about this.
The current approach of the MR is not ideal:
- All the HTMX-specific helpers and documentation lives in the
\Drupal\Core\Templatenamespace - Values are transformed to an AttributeValueBase right away, making it unusual to work with in alter/preprocesses
Thankfully we can fix it by moving things around:
- Move the builder object to the
\Drupal\Core\Htmxnamespace - Make it work more like
CacheableMetadata, with anapplyTo()andcreateFromRenderArray(we need this to unserialize the json attributes) methods that we call on a render array to augment it. - In the applyTo method, save all the data as attributes in the render array
#attributeskey, and json_encode what needs to be json_encoded and let the existing code handle the attribute rendering
we don't need the page wrapper, the ajax framework doesn't have it and this shouldn't either, offcanvas, skiplink are not relevant for an ajax response
We need the empty URL thing for the post/patch/delete/put methods too
nod_ → created an issue. See original summary → .
Opened a MR with only the changes we converged on, I kept the swap-oob true, I could be convinced to do it like before but I don't want to keep track of css selectors in the backend.
Added a small condition so that form_build_id doesn't get the htmx attribute when its not an htmx request.
it's not actually postponed.
Looks like the only big difference is the configexport form, which probably shouldn't be part of this issue so we can discuss the approach on how to htmx-ify the forms without holding up the necessary pieces in this MR
few cosmetic feedbacks
seems the CI is not happy
very nice. I couldn't make the config export form work on my end. I have no idea why setting the data-hx-swap-oob earlier makes the whole thing work… There is a side effect somewhere and couldn't figure out what it is yet.
I think the changes to the form are a bit worrying. We can't expect people to make this kind of changes to switch from ajax to htmx. I guess that's a concern for once things actually work.
Just realized we're going to have troubles with inline form errors, we need to swap the whole wrapping element, not just the select when it's updated.
Committed and pushed 5a125053a45 to 11.x and 6a46e4895c6 to 11.2.x. Thanks!
we need 📌 Return htmx responses as SimplePageVariant Active before this works properly
I'd drop it. We can always add a followup to add it and we can figure out the problems without holding the working part of the MR
For the _wrapper_format query string solution I have a MR up at https://git.drupalcode.org/project/drupal/-/merge_requests/12900
Bunch of updates, implements the _wrapper_format=drupal_htmx to reduce the size and contents of the ajax requests
Committed and pushed d6f8e99c4cd to 11.x and 9271a55faa8 to 11.2.x. Thanks!
I'd rather use conditional chaining for this.
document
.querySelector('#toolbar-administration')
?.classList.add('toolbar-oriented');
Haven't tested the UI but I'm ok with the clever solution. Maybe we can name it #extra-specificity-hack? to make sure we're not saying it's a supported pattern, and as a callback to old school CSS hacks :D
some comments
It is not going to be a drop in replacement see ✨ Replace ajax frontend implementation with htmx Active . Trying to be backwards compatible is not reasonable. We tried that a few years ago with jQuery UI and autocomplete. Today autocomplete is still jQuery UI.
Going with htmx does mean we loose some separation of concerns for locality of behavior: https://htmx.org/essays/locality-of-behaviour/ Where we draw the line is necessarily going to change.
Can we change all the similar selectors in the method? otherwise we'd wonder why this one is using css selector and it's using named selectors above.
Among other things, thank you for your help removing the overlay!
Thanks @godotislate :)
I'd like someone to test the scenario described in the comment being removed ? Not breaking forms is more important than being valid HTML in this case.
Committed 004cf1e and pushed to 11.x. Thanks!
nod_ → made their first commit to this issue’s fork.
Don't think this needs postponing
Please be careful to not publish the change record if the issue is not fixed.
Yes there are (at least) two strategies :D The needs XB addresses are not the same as what we're addressing with HTMX.
XB is yet to be stable, and I don't know about the stability of the React components but let's assume it's not going to take over the Drupal ecosystem on day 1 :) if you want you can think of HTMX as an intermediary step to simplify the backend, DX, and reduce the footprint of the existing JS everyone in Drupal currently uses. It's more than that but that alone is a good argument for it.
We have already reduced the weight of bigpipe thanks to HTMX. Do we need React to implement Views pagination or ajax forms? I would say no, so moving away from jQuery and unsupported libraries (jQuery form) is something worthwhile to work on. We still have to deal with what people use today and we can make it better for developers and maintainers.
Thanks! can you remove Théodore 'nod_' Biadala (feed) too? or a new issue is better?
we should use the Caddyfile from
📌
Add Caddyfile configuration
Active
and frankenphp run.
I have some setup steps maybe that could help see how it's running? https://tresbien.tech/blog/drupal-and-php-fun-frankenphp/
Just published a second article :)
fair enough, let's go with that
I see that makes sense.
I'm not super happy with the negation, can we have something clearer, it's not obvious that UI = display builders, no_ui could mean the component only output hidden html (script tags) but we do want that to be available in the interface to put on the page? I'm all for consistency but is there a big overlap of folks writting SDCs and FieldTypes/views plugins that needs to be hidden?
I asked some AIs for the naming, got a few interesting suggestions: uiHidden (clearer that the SDC has an output, just we don't want to list it), excludeFromUiBuilders longer but can't really argue about the clarity.
Even got headless as a suggestion since it's for programmatic use, not saying we should use that but it's an interesting take on the situation :)
So we have:
"no_ui": {
"type": "boolean",
"title": "Internal",
"description": "Use this property to exclude a component from the UI, for internal components."
},
Why not call it internal directly? We already have a sort of internal prefix for libraries we don't want people to rely on, that would solve the camelcase issue too
fjgarlin → credited nod_ → .
added test
Feedback in MR
Let's do follow-ups, I'd rather get that in regularly than wait 6 months for a big MR.
Committed 93279d3 and pushed to 11.x. Thanks!
The change is reasonable, CR is clear too.
One question on the MR.
Committed and pushed fa5980671e2 to 11.x and 09b2afc5921 to 11.2.x and 036463a2eb7 to 10.6.x and 1ed197bc9df to 10.5.x. Thanks!
good to go, thanks
Committed and pushed 7cf609309d5 to 11.x and ac36d01e3f1 to 11.2.x. Thanks!
Committed 2732a4f and pushed to 11.2.x. Thanks!
We need a MR for 10.6.x and below. Cherry pick isn't clean
Committed and pushed 72c39ca98ec to 11.x and dce32095c10 to 11.2.x and 0f3a62ab851 to 10.6.x and f0525143a35 to 10.5.x. Thanks!
MR is good to go, The documentation has been read and understood.
Committed 9031a4e and pushed to 11.x. Thanks!
Committed and pushed eb0e5f464c5 to 11.x and da5cd09f6f9 to 11.2.x and ec78aecacb1 to 10.6.x and 95d051ff301 to 10.5.x. Thanks!
very much a +1 ! You've been great to work with
We already have a test with multiple body tag, that's why I added the check to the value of the hitTheFloor variable, see test-only CI run: https://git.drupalcode.org/issue/drupal-3533698/-/jobs/5753271
Thanks for keeping things in check!
@fathershawn you didn't misunderstand, I think I didn't explained it clearly enough.
My initial plan was quick and dirty: do the replacement of the ajax commands only so we can get that done quickly and move on. When you added your MR that went with a more HTMX way of doing it, it looked interesting: changing ajax commands for a meta header and raw HTML looked like a great improvement, and you were really involved in making it work so I though it'd be good to try it out. It started to get hard with asset handling and messages. To make it work in that new way the changes and compromises started to become too much for comfort. After chatting with larolwan and catch we figured the less user-disrupting way to go about it was to do the initial quick and dirty implementation. That way if something breaks, we know it comes from a mismatch between what we expect and how htmx does things, because it's the only thing we changed.
Bigpipe is not broken, if we change the data format, how the data is sent or processed we risk breaking it in ways that are not obvious. Check out the history on the big_pipe.js file, there has been many fixes after we moved to the mutation observer handling, it took many years to find the bugs and fix them. I don't want to add more work on this area when there are other things broken we could work on.
Dealing with how messages are handled is going to be easier and less stressful when the change is by itself, not blocking a big feature MR. I second @godotislate assessment the smaller the diff the more likely it is to get committed quickly.