feat: Add `data-ujs-morph-root` for morphdom rendering

taltas commented 2 years ago

Status

Needs review

Additional Notes

Add in a new data attribute called "ujs-selector' which is a string that is used to query the page to find an element . If the element is found it will be subjected to morphdom, if it's not found, default to document.body (as before). This is very handy for updating the DOM with partials.

I'm wondering whether it would be worth addding a couple of additional data selector attributes for success vs failed response as opposed to a universal selector data attribute. What do you think?

netlify[bot] commented 2 years ago

Deploy Preview for mrujs ready!

Name	Link
Latest commit	7e8b44262eb6c2c254507ad9bf3aef7b8df240dc
Latest deploy log	https://app.netlify.com/sites/mrujs/deploys/624cf178341f270008c67b87
Deploy Preview	https://deploy-preview-187--mrujs.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

KonnorRogers commented 2 years ago

@taltas so this would only apply to failures. All successes get piped through turbo / turbolinks so they dont apply morphdom so thered be no selector to use.

KonnorRogers commented 2 years ago

so if i understand correctly you add data-ujs-selector to the <form> and it will run a search for the element, and default to morphing the body?

taltas commented 2 years ago

@taltas so this would only apply to failures. All successes get piped through turbo / turbolinks so they dont apply morphdom so thered be no selector to use.

Hmmm, if you submit a form and redirect the page you were originally on, it will still try and use the 'ujs-selector' on success from what I tested, i.e. it will fill the selector div with the entire redirected page's body. I'm curious as to why there is the isSamePage check to do the morph here, rather than letting turbo deal with the same page (I'm not really sure how Turbo handles this scenario)?

If there is no way around this, I'm wondering whether the ujs-selector should only be for when the response is an error reponse, otherwise use the normal body morph, perhaps renaming the attribute ujs-error-selector ?

taltas commented 2 years ago

so if i understand correctly you add data-ujs-selector to the <form> and it will run a search for the element, and default to morphing the body?

Basically yes, except see the one issue when the response is successful from the server.

taltas commented 2 years ago

@ParamagicDev Just giving this a bump

KonnorRogers commented 2 years ago

@taltas so the reason that isSamePage is there is for it to do a pushState if someone is using Mrujs without Turbo / Turbolinks. Give me a little bit to work on this. The navigationAdapter is admittedly not super well tested and is quite thorny, I'd like to add some tests to this. (I generally don't have too much time during the week, I tend to do most of my OSS on weekends)

taltas commented 2 years ago

@taltas so the reason that isSamePage is there is for it to do a pushState if someone is using Mrujs without Turbo / Turbolinks. Give me a little bit to work on this. The navigationAdapter is admittedly not super well tested and is quite thorny, I'd like to add some tests to this. (I generally don't have too much time during the week, I tend to do most of my OSS on weekends)

No worries, just wanted to make sure you saw it more than anything. :+1:

KonnorRogers commented 2 years ago

@taltas How would you feel about calling this data-ujs-morph-root?

If you do this:

<form data-ujs-morph-root></form>

It will mean "only morph the form submitting"

If you do this:

<div class="class">
  <form data-ujs-morph-root=".class"></form>
</div>

It'll morph everything in the div, how does that sound to you?

taltas commented 2 years ago

@taltas How would you feel about calling this data-ujs-morph-root?

If you do this:
<form data-ujs-morph-root></form>
It will mean "only morph the form submitting"

If you do this:
<div class="class">
  <form data-ujs-morph-root=".class"></form>
</div>
It'll morph everything in the div, how does that sound to you?

I think morph-root makes more sense as that is what it's actually doing. Are we still going to target for success or fail? or only fail?

KonnorRogers commented 2 years ago

@taltas we could pass it to both, but it would require opting out of Turbo based navigation which isnt ideal. But for those not using Turbo, it should work.

taltas commented 2 years ago

Actually, I think leaving it as is right for now. I think the morphDOM when it hits the same page is a seperate issue in itself. I think this PR is good to go.

KonnorRogers / mrujs