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

pjax form submit heroku example #489

Open
wants to merge 11 commits into
base: heroku
Choose a base branch
from

Conversation

hiroshi
Copy link

@hiroshi hiroshi commented Feb 19, 2015

https://pjax-form.herokuapp.com/

I just add a form to the heroku example branch. I think it help someone want to try pjax form submit.
Current status is let it just work. To be merged there still some work needed.

  • Add a pjax form with a text input. Value of the input will display.
  • Update jqeury.pjax to use $.pjax.submit.
  • Replace old local jquery src with new CDN src. Because latest jquery.pjax require newer jquery.
  • Replace or remove old jquery file (if use of a CDN doesn't seem to be suited).
  • jquery.cookie.js contained in the branch does not work with new jquery. replace with new one, if available, or use an alternative, or ..... -> sessionStorage!

@mislav
Copy link
Collaborator

mislav commented Mar 2, 2015

Thanks for the submission. What were you trying to do here?

@hiroshi hiroshi changed the title pjax form submit [WIP] pjax form submit heroku example Mar 3, 2015
@hiroshi
Copy link
Author

hiroshi commented Mar 3, 2015

What were you trying to do here?

Sorry, I added description to the first comment.

I just want to see $.pjax.submit works. But heroku example doesn't have form example so I added.
I want to share this with my team mates. I think creating PR is best to do. Also I think this PR will help someone.

@mislav
Copy link
Collaborator

mislav commented Mar 3, 2015

Gotcha. Well, your pull request is based on a commit that's very old: from year 2012 it seems. Then you upgraded the library files in the commit "Update jquery and jquery.pjax.js". That wasn't the proper way to upgrade these files. A better way would be to base your changes on a newer commit, preferrably last commit on our master branch. A way to update this pull request would be:

  1. git fetch origin master (assuming "origin" remote points to defunkt/jquery-pjax)
  2. git checkout heroku-submit — switch to the branch where you're doing work on this PR
  3. git rebase -i origin/master — interactive rebase
  4. During the interactive rebase, only pick to keep the "pjax form submit" commit, and discard the "Update jquery…" commit.
  5. git diff origin... should now contain only your form-related changes
  6. git push -f — force-push the "heroku-submit" branch to your remote, and this pull request will automatically get updated with newest changes.

@hiroshi
Copy link
Author

hiroshi commented Mar 4, 2015

Well,
It seems that the heroku branch is an orphan like gh-pages.
See first commit of the branch 88b7074 has 0 parents.
And latest "update pjax" commit 64ff5ce has 1 parent. @defunkt just overwrote jquery.pjax.js from master. So I thought I do same thing...

Anyway I tried your instruction...

git fetch origin master
git checkout heroku-submit
git rebase -i origin/master

pick 88b7074 first draft
pick aa45e93 hook it up
pick 636006b home n whatnot
pick bf9c0f0 sweet
pick c2460f0 better explanation
pick 8044633 make errything work
pick 00a7f5a finishing touch
pick 914b638 favicon
pick e1e376e cleaner, better
pick 4e5454d readme
pick 1bf695f view
pick f00abaf no pjax for you :(
pick 23cb50b load
pick e2c656e update from master
pick 6ea1d31 update pjax
pick c2a7265 update pjax... again!
pick c7f80c5 iOS web apps <3 pjax
pick 64ff5ce update pjax
# pick 7b152cf Update jquery and jquery.pjax.js.
pick a0d7c26 pjax form submit.

# Rebase 56b950b..a0d7c26 onto 56b950b
#
...
error: could not apply 88b7074... first draft

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 88b7074a0735f930e5bbf725abd7c8c35ecdb625... first draft

Hmm,
I think I cannot rebase branches without same origin. or I did something wrong way?

@mislav
Copy link
Collaborator

mislav commented Mar 4, 2015

Oh I'm sorry, I provided you with wrong instructions. I thought that you opened a PR for our "master" branch, like most of our PRs. But you made a perfectly valid pull request for our "heroku" branch which was indeed slightly out of date because it only serves as a demo app. Ignore my above instructions.

<script src="jquery.cookie.js"></script>
<script src="pages.js"></script>
<!-- <script src="jquery.js"></script> -->
<script src="//code.jquery.com/jquery-1.11.2.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from loading from CDN. You may remove the old vendored jquery.js and references to it.

@hiroshi
Copy link
Author

hiroshi commented Mar 5, 2015

@mislav Thanks for your review.
Yeah, currently my change are kind of quick hacks. I'll fix those where you point to be capable to be merged.

@hiroshi hiroshi changed the title [WIP] pjax form submit heroku example pjax form submit heroku example Mar 8, 2015
@hiroshi
Copy link
Author

hiroshi commented Mar 8, 2015

@mislav I updated most of codes where you pointed. Also updated https://pjax-form.herokuapp.com/.

if ( !$(':checkbox').attr('checked') )
$.fn.pjax = $.noop
if ( !$(':checkbox').prop('checked') )
$.fn.pjax = $.pjax.submit = $.noop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead do $.pjax.enable/disable() based on the state of checkbox?

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.

2 participants