iLoveElysia/pjax
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

prevent scrollTo from being converted from false to 0 #33

Merged
igierard merged 3 commits from disableable-scrollTo into master 2015-01-16 00:56:25 -05:00
Conversation 16 Commits 3 Files Changed 3 +112 -37
igierard commented 2014-11-18 17:46:43 -05:00 (Migrated from github.com)
Copy Link

In the loadUrl function there is an explicit check for false as a means to disable the scrollTo behavior however if the scrollTo option is passed to the constructor as false the gaud statement was converting false to 0.

In the loadUrl function there is an explicit check for false as a means to disable the scrollTo behavior however if the scrollTo option is passed to the constructor as false the gaud statement was converting false to 0.
MoOx commented 2014-11-19 02:12:14 -05:00 (Migrated from github.com)
Copy Link

I'm currently refactoring this lib, can you please provide at least a test for this ? Thanks !

I'm currently refactoring this lib, can you please provide at least a test for this ? Thanks !
igierard commented 2014-11-20 14:12:12 -05:00 (Migrated from github.com)
Copy Link

Are you changing the public api (including such as changing what attributes are available) or just refactoring internals? My main reason for asking is that my test for this would require reading .options of the Pjax object? Also is master where this development is taking place?

Are you changing the public api (including such as changing what attributes are available) or just refactoring internals? My main reason for asking is that my test for this would require reading .options of the Pjax object? Also is master where this development is taking place?
MoOx commented 2014-11-20 14:16:22 -05:00 (Migrated from github.com)
Copy Link

You can fork current master that is WIP right now. Take a look to how test are written, you should not need an pjax instance because we should explode the rest of index.js into smaller entities.
You can eventually take a look to the git history to see how I'm doing that.
If this looks to complicated for you, keep this PR open & I'll handle that, but I don't know when :)

You can fork current master that is WIP right now. Take a look to how test are written, you should not need an pjax instance because we should explode the rest of index.js into smaller entities. You can eventually take a look to the git history to see how I'm doing that. If this looks to complicated for you, keep this PR open & I'll handle that, but I don't know when :)
igierard commented 2014-11-20 19:25:11 -05:00 (Migrated from github.com)
Copy Link

Test added. I had to pull the options defaults into a separate file so as to not instantiate a new pjax object. (it is basically only an issue instantiation). The only thing failing in int npm test was failing before as well.

Test added. I had to pull the options defaults into a separate file so as to not instantiate a new pjax object. (it is basically only an issue instantiation). The only thing failing in int npm test was failing before as well.
MoOx commented 2014-11-24 03:38:23 -05:00 (Migrated from github.com)
Copy Link

Thanks for this.
I'll take a look when I will continue this refactoring (I'm working hard on cssnext right now).
Sounds good tho.

Thanks for this. I'll take a look when I will continue this refactoring (I'm working hard on cssnext right now). Sounds good tho.
igierard commented 2014-12-31 13:45:00 -05:00 (Migrated from github.com)
Copy Link

Any word on this?

Any word on this?
MoOx commented 2015-01-07 11:58:47 -05:00 (Migrated from github.com)
Copy Link

No time for this for now. Are you interested to get write access ?

No time for this for now. Are you interested to get write access ?
igierard commented 2015-01-12 14:14:40 -05:00 (Migrated from github.com)
Copy Link

If you're comfortable with that I'm up for it.

If you're comfortable with that I'm up for it.
MoOx commented 2015-01-12 15:32:00 -05:00 (Migrated from github.com)
Copy Link

You should now have full access to the repo. Be careful ;)
What is your npm username if you have any (otherwise I will make the release when needed)?

You should now have full access to the repo. Be careful ;) What is your npm username if you have any (otherwise I will make the release when needed)?
MoOx commented 2015-01-16 00:56:21 -05:00 (Migrated from github.com)
Copy Link

lgtm, merging so we can move forward

lgtm, merging so we can move forward
igierard commented 2015-01-16 17:40:21 -05:00 (Migrated from github.com)
Copy Link

Sorry I didn't get on that faster had another project cannibalize my time for a bit.

Sorry I didn't get on that faster had another project cannibalize my time for a bit.
igierard commented 2015-01-16 17:41:08 -05:00 (Migrated from github.com)
Copy Link

Also interestingly write access doesn't seem give you the ability to merge pull requests.

Also interestingly write access doesn't seem give you the ability to merge pull requests.
MoOx commented 2015-01-17 08:16:28 -05:00 (Migrated from github.com)
Copy Link

Where did you see that?
(I merged your PR).

Where did you see that? (I merged your PR).
igierard commented 2015-01-19 15:14:25 -05:00 (Migrated from github.com)
Copy Link

After you gave me write access I looked at the pull request and there was no option to merge it in. At least not that I could find. Might have something to do with the fact that I submitted it. Thanks for the merge anyway. I submitted a few bug fixes also as just normal commits. Wanted to let you know but didn't know where to message you at.

After you gave me write access I looked at the pull request and there was no option to merge it in. At least not that I could find. Might have something to do with the fact that I submitted it. Thanks for the merge anyway. I submitted a few bug fixes also as just normal commits. Wanted to let you know but didn't know where to message you at.
MoOx commented 2015-01-19 15:22:09 -05:00 (Migrated from github.com)
Copy Link

I merged your PR before you take a look (when I give you write access).
As a collaborator, you can create branch into this repo directly (no need for a fork) & make a pull request + merge. By doing that, you offer other collab to make a review (even if you merge directly) of what have been merged. Nice way to notify collaborators.
Or you can just ping me (like "poke @MoOx") in commit message (in what is considered as a description (below the first line)).

I merged your PR before you take a look (when I give you write access). As a collaborator, you can create branch into this repo directly (no need for a fork) & make a pull request + merge. By doing that, you offer other collab to make a review (even if you merge directly) of what have been merged. Nice way to notify collaborators. Or you can just ping me (like "poke @MoOx") in commit message (in what is considered as a description (below the first line)).
MoOx commented 2015-01-19 15:23:58 -05:00 (Migrated from github.com)
Copy Link

Just looked at your commit. You are doing a great job. Thanks !

Just looked at your commit. You are doing a great job. Thanks !
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
dependencies
Pull requests that update a dependency file
 duplicate
enhancement
invalid
question
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
iLoveElysia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#33
Delete Branch "disableable-scrollTo"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?