Skip to content

Conversation

@chinaowl
Copy link

@chinaowl chinaowl commented Aug 9, 2016

Hi @ejucovy and Matthew!

This is a demo of the ActionKit popup form that we developed. To run the demo, just open index.html in a browser.

Feedback welcome!

And thanks to @kprakobkit and others for contributing!

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2016

CLA assistant check
All committers have signed the CLA.

@ejucovy
Copy link
Contributor

ejucovy commented Aug 9, 2016

Very cool!!

  • When using ajax mode, I don't think the form submission is actually working? If you look at network requests in a debugger while submitting that request, you'll see that the response contains validation failures because the request is submitting extra (blank) copies of all the form fields -- it looks like there are actually two forms in the document matching the $("form[name=actionkit-form]") selector, I'm guessing because of this line in modal.js - var modalContent = $(modalSourceID).clone(true);. That $('form[name=actionkit-form]').serialize(); should probably be changed to $(this).serialize() or something, to avoid this, and to make sure that it works even if there are multiple popups on a page.
  • Also, I don't think $.ajax({"dataType": "jsonp", "type": "post"}) actually means anything? (A jsonp request is always GET.) Would prefer the use of that ak-async-post.js library I mentioned in chat, though a simple jsonp GET works too (just sort of scares me)
  • Per slack chat - when using data-actionkit-behavior="redirect", a target attribute on the anchor should be respected, so that <a target='_blank' data-actionkit-behavior='redirect'> will open the redirect in a new tab, etc. I think this should just be a matter of checking for a target attribute on the anchor and stuffing it onto the generated form if present, since forms also support target attributes.
  • The popup forms are extending above the top of the browser window for me - are you seeing that too? Is there an easy way to limit the height of the popup and provide independent scrolling within it, assuming very long content? Or some other way to ensure that the top of the content isn't above the top of the window? Or is this just demo css that we don't need to worry about?
  • Handling error responses from actionkit would be nice in ajax mode, and running clientside validation to catch errors before submission (using the strings provided in that /context/ api call) would be extra nice. :-) Not essential though.

I'll keep trying this out on some other pages too - I suspect some page types and/or some languages will turn up edge cases.

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants