Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal about forms #478

Open
PaulMaly opened this issue Apr 3, 2018 · 9 comments
Open

Proposal about forms #478

PaulMaly opened this issue Apr 3, 2018 · 9 comments

Comments

@PaulMaly
Copy link
Contributor

PaulMaly commented Apr 3, 2018

Hi, @matthewp! I've an idea:

<form method="GET" action="/products">
   <input name="q" type="search" placeholder="Search...">
   <button>Search</button>
</form>

the result of this form submissing will be URL changes:

/products?q=Macbook

So, maybe it's a good idea to provide an option for handling GET-forms by the router? Just like we already handle all links. It could be something like this:

page.start({
   click: true,
   submit: true
});

I can provide a PR for these changes if you don't mind.

@matthewp
Copy link
Collaborator

matthewp commented Apr 3, 2018

Sounds good to me!

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Apr 3, 2018

Great! I'll send a PR.

Btw, are you sure that this:

  /**
   * To work properly with the URL
   * history.location generated polyfill in https://github.com/devote/HTML5-History-API
   */

  var location = isWindow && (window.history.location || window.location);
  var isLocation = !!location;

was need to be changed to this:

  /**
   * To work properly with the URL
   * history.location generated polyfill in https://github.com/devote/HTML5-History-API
   */

  var isLocation = hasWindow && !!(window.history.location || window.location);

Seems, that it's not an equivalent change and we've lost HTML5-History-API polyfill support, isn't?

@matthewp
Copy link
Collaborator

matthewp commented Apr 3, 2018

Why is it not equivalent exactly?

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Apr 3, 2018

Because the idea was to rewrite local location variable to window.history.location if this polyfill is used. Not just to check that we've location. Correct me if I'm wrong.

@kethinov
Copy link
Contributor

kethinov commented Apr 3, 2018

Something similar to this was discussed in #107 and the previous maintainer declined to accept pull requests on the subject. I went ahead and implemented it as a plugin instead: https://github.com/kethinov/page.js-body-parser.js

Though I haven't tested either of my two page.js plugins since this project became actively maintained again, so they may need some maintenance to get working again.

Also for what it's worth if the new maintainers are interested in reconsidering the issue, I still think it would be neat to have this feature included in page.js itself rather than needing a plugin to add full support for handling forms.

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Apr 3, 2018

@kethinov Hi!

Hm, I believe it's a quite different thing. In this issue, you suggest handling of POST-forms, right? I'm not sure that it's a bad idea or something, but in a difference of GET-forms, POST-form also sends a request body to the server. And I think that body processing isn't business of a router.

GET-forms and it's processing is very simple - they change URL, as well as any regular link does it. I think processing of such things like query-string and request bodies could be taken out in plugins.

@kethinov
Copy link
Contributor

kethinov commented Apr 3, 2018

Yeah, that is the correct distinction. Personally I do think handling the body should be in scope of the router (for both POST and GET), but the consensus was against it before, so I'm not gonna be super broken up about it if the consensus is against it again. 😉

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Apr 3, 2018

Actually, I'm not really against it. )) But seems even query string is not processing in Page.js by default. To stay tiny, I guess. For me, it's not a problem to import qs or something and write 3 loc middleware to support it.

Anyway, your idea deserves attention, so maybe you should raise an issue again to check whether the consensus has changed?

@PaulMaly
Copy link
Contributor Author

PaulMaly commented Apr 4, 2018

PR: #480

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

No branches or pull requests

3 participants