Closed FilipNest closed 8 years ago
Our code changes will clash a bit here as i've pulled out the template.onSubmit function into the clientside.js file.
It feels wrong to be inserting duplicated js snippets into the page markup. For every form on the page, it inserts the same snippet into the page. My way, the snippet only appears once in a static js file which is where it should be.
This is definitely a good feature to have though, i think it will help with theming too as we could try and use the same handlebars templating for forms/fields instead of having to introduce an extra templating language with underscore.
Currently every field on a page has to be themed the same way as the templates are global, however if you want textfields, or buttons or whatever to themed one way on form x, and a different way on form y, its not currently possible. With server side rendering we can hopefully have more fine-grained control of the output.
My issue yesterday was that I thought we couldn't access the jsonform object from a different js file, but thats because I was trying to access 'jsonform' instead of 'JSONForm'. We can hopefully get away without having to fork the library
Moving the common onSubmit handler to clientforms.js is a good idea as long as it can be overriden if you put one in yourself for your form (as some will want to override it).
It shouldn't clash with this as that onSubmit handler isn't even called by the server side forms. They're just static-rendered.
This doesn't fix the theming issue as that's something different. Worth looking into perhaps but this pull request is more straightforward. It's just to get forms working without JavaScript. Does open up lots of possibilities. For theming I'd use custom field templates which JSONForm supports fairly well.
All this is doing is exactly the same JSForm stuff on the server that happens on the client side. Instead of replacing the form completely though (as JSONForm may have event handlers bound to elements rather than class names) it removes the static one if JavaScript is enabled and replaces it with the dynamic JSONForm one.
It might be a tricky merge if we're dealing with similar files but I don't think they're the same bits of those files.
I've had a look through and i'm not loving the need to add dynamicForm = true to certain forms. I think it could cause a lot of confusion and frustration as it could be forgotten, or forms change with time or forms get altered elsewhere etc. There will be a lot of 'whys it not doing what it should' type frustrations.
We need to think of a way around this, one possibility is to render forms server side, then just re-render client-side where js is available as a rule, so that events can be added where ever they are needed. If js is not available, it will just show the server-side rendered form. Or try to be clever and asses the form array to determine automatically if js would be required on the client-side
We need to think of a way around this, one possibility is to render forms server side, then just re-render client-side where js is available as a rule, so that events can be added where ever they are needed.
That's exactly what this is doing.
I think I've caused some confusion as to what the dynamic forms variable does, it shouldn't be needed the majority of the time, I only put it in at the last minute. I've also realised my bit about stopping database queries is not true as they usually happen when the form object is being generated so this is irrelevant.
Here's what happens:
All dynamic forms should still work as they're re-rendered with JavaScript on the client side just like they always were. The reason we need to do this is that JSONForm will bind to specific DOM events rather than pushing in onclick etc handlers into every HTML element.
The dynamicForm variable is there to tell an end user that the form is completely unusable without JavaScript so if they disable JavaScript, instead of showing a form that has all sorts of essential autocompletes and things (this should be very rare and avoided if possible) it will show a "Please enable JavaScript" type message. This server-side rendering is only intended as a fallback for people who don't use JavaScript. The form rendering we've had in the past is still used whenever JavaScript is enabled on the client side. Ideally we'd render the whole form server side including event handlers but I don't think that's possible, unless we hack into JSONForm a lot.
Does that make a bit more sense?
The files have changed a lot this week since this was made so I can't fix these merge conflicts easily. I suppose it'll have to be held back a week. Shame.
This is still waiting for conflicts to be fixed
Yeah, I'm going to take another stab at it when I get a moment. Feel free to ignore it until I get there.
Support no-js loading of forms through server-side rendering. In order not to break any dynamic JSONForm handlers, if client-side JavaScript is enabled, the server side form is replaced with the normal JavaScript-enabled one.
Some forms require JavaScript so to not allow the form to have a JavaScript-less version (also useful to skip the server side rendering if the form contains a lot of database queries or other processor intensive tasks), and instead display a translatable
<noscript>
message, you can pass through thedynamicForm = true
parameter in your form object.Simple forms, including our custom fields work but a nojs test on the entity form caused an error on the submit handler (form rendered correctly otherwise so it's probably fixable). This is more for simple forms on the client side than admin tasks but with some tweaks we should be able to get most forms working.
Redirect callbacks work, as do error messages which are translated into a redirect and iris.message rather than the client side JavaScript messaging of the JavaScript powered forms.
Note: Requires a NPM install for the JSDom library.