Added html5 history support on ajax requests on CGridView and CListView #444

Merged
merged 15 commits into from Mar 5, 2012

Conversation

Projects
None yet
7 participants
Contributor

lightglitch commented Mar 3, 2012

No description provided.

Owner

samdark commented Mar 3, 2012

Good usability enhancement. @qiangxue, @mdomba what do you think?

Member

mdomba commented Mar 3, 2012

Seems fine to me, just to fix those minor things I commented...

Contributor

lightglitch commented Mar 4, 2012

I think everything is fixed now.

Member

mdomba commented Mar 4, 2012

Did you test this with nonHTML5 browsers to see if the hash fallback works with the Yii code?

If not would be good to disable the fallback functionality.

Contributor

lightglitch commented Mar 4, 2012

I tested on IE and the hash fallback is not implemented.

 // Check to see if History.js is enabled for our Browser
 if ( History.enabled ) {
Contributor

lightglitch commented Mar 4, 2012

There is a bug don't merge yet.

Contributor

lightglitch commented Mar 4, 2012

I had implemented this the wrong way please recheck the latest commits.

I also removed the html4 hash fallback because it would need some special handling when you open a page with parameters in the hash

Contributor

lightglitch commented Mar 4, 2012

Finally, I hope I have fixed all the formatting issues.

Member

mdomba commented Mar 4, 2012

Seems fine, @qiangxue any thought on this ?

Member

mdomba commented Mar 4, 2012

@cebe is there a way to test a pull request without merging?

I guess I would need to clone the user branch for that, any easier/faster way?

Owner

qiangxue commented Mar 4, 2012

I'm not familiar with this. Does jquery.history.js make window.History available? My only concern is whether History.enabled may cause trouble for some browser.

Contributor

lightglitch commented Mar 4, 2012

Just to clarify window.History is the plugin, the History API is window.history.

@mdomba: you can just download the patch https://github.com/yiisoft/yii/pull/444.patch and apply it just for testing.

Contributor

phpnode commented Mar 4, 2012

@mdomba for future reference, the fastest way is to add the contributor's fork as a remote, then checkout their relevant branch, so in this case:

git remote add lightglitch https://github.com/lightglitch/yii.git
git fetch lightglitch
git checkout lightglitch/gridview-url

You only need to do the first line once per contributor

Member

mdomba commented Mar 5, 2012

Sorry if I'm making you "polish" the JS code too much... but as this is a framework and many users will be using it and read the code, IMO it's good to pay attention to those small things so that the code is not only functional but even styled as it should.

There are few things to fix

jquery.yiigridview.js:

  • line 13 has one space at the end of line - should be removed
  • line 15 has 33 spaces, should be just a newline
  • line 88 has 8 TABs, should be just a new line
  • line 107 same as line 88
  • line 120 has 5 TABs after }, they should be removed
    jquery.yiilistview.js
  • line 13 has 9 spaces
  • line 42 has 7 TABs
  • line 57 has 5 TABs after }
Member

mdomba commented Mar 5, 2012

You should add a check for ajaxUpdate too as a user can set this to false to prevet ajax requests.

Owner

cebe commented Mar 5, 2012

@mdomba I'd say what @phpnode suggested is the best way to do this.

Contributor

lightglitch commented Mar 5, 2012

No problem it's fixed now.

mdomba pushed a commit that referenced this pull request Mar 5, 2012

Merge pull request #444 from lightglitch/gridview-url
Added html5 history support on ajax requests on CGridView and CListView

@mdomba mdomba merged commit 1984270 into yiisoft:master Mar 5, 2012

Member

mdomba commented Mar 5, 2012

Great, thanks...

Member

mdomba commented Mar 5, 2012

Hmm.. as I merged this I realized that this is "forced"... so that the users cannot decide if they want this behavior or not
What if someone don't want this functionality ?

@qiangxue, @samdark

Does it make sense to add an option for this and to default to false to be BC ?

Owner

qiangxue commented Mar 5, 2012

Yes, I think we should make this an optional feature and should be turned off by default.

Contributor

lightglitch commented Mar 5, 2012

Well, I don't see any reason why anyone wouldn't want this behavior.

Just changes the urls and if you click on the view or edit icon and then use the back button the grid shows with the latest state.

It's like simulating the non ajax behavior.

Contributor

phpnode commented Mar 5, 2012

I'm concerned about what happens when you have 2 CGridView or CListViews on one page.
Lets say I have a user grid and a books grid, I've not tested it but I suspect what will happen is:

  1. I click page 2 on the user grid, the url changes to /foo/?User_page=2 but the page is not reloaded
  2. I click page 4 on the books grid, the url changes to /foo/?Book_page=4
  3. This is not quite the expected behavior, the url should probably change to /foo/?User_page=2&Book_page=4, but I suspect that is quite difficult to achieve

Contributor

lightglitch commented Mar 5, 2012

@phpnode: Never thought about several grids in one page in that case it's problem.

Contributor

phpnode commented Mar 5, 2012

@lightglitch i think as long as you make this feature optional as @qiangxue suggests, your solution is still good for 90% of use cases, and an acceptable tradeoff for the other 10%

Owner

qiangxue commented Mar 5, 2012

It's possible a single page may contain multiple grids, and we should take care of this situation.
By introducing a switch to turn this feature off by default, it also makes us less worried about potential BC breaking issues (just like this multiple grids case.)

Contributor

phpnode commented Mar 5, 2012

@qiangxue - very hard to solve the problem properly because you have to do url parsing and creation client side, i had a go at this already in my YiiJS experiment, but i'm not sure we want this in the core, https://github.com/phpnode/YiiJS/blob/master/js/yii/web/CUrlManager.js

Contributor

lightglitch commented Mar 5, 2012

Just one question: what is the behavior if you have multiple grids in one page but all have the AJAX update turned off?

Contributor

phpnode commented Mar 5, 2012

@lightglitch - the url will update correctly, so from my example above:

  1. /foo
  2. /foo/?User_page=2
  3. /foo/?User_page=2&Book_page=4
Member

mdomba commented Mar 5, 2012

in any case we should add the option as if some user don't want this feature we should not force it, even if it's just a url change... maybe someone do not like to have all those parameters in the URL or any other reason... and @phpnode is right I forgot to consider the page with more then one grid or a grid with a listview... so this case should be documented very good under the new option description so that it's clear for the users that by enabling this option it will work only with one grid on the page

@qiangxue you are the best for parameter naming, as this works only on html5 browsers maybe call this option "html5history" ?

@cebe, @phpnode - it's not needed for now, but just to learn those operations... how would I now reverse this merge ? I guess there is no "one click on github" for this.

Contributor

lightglitch commented Mar 5, 2012

@phpnode If it works for non ajax should also work in this case I will do some testing latter on.

Contributor

phpnode commented Mar 5, 2012

@mdomba I think the best way to revert a merge is to run the following command:

git reset --hard HEAD^

Which takes you back one commit. Repeat until you get to the commit you want to restore to.
Then, to push to master, run the following:

git push --force origin master

the --force stops git from complaining that your branch is out of date. @cebe please correct me if I'm wrong, it's been a while since i did this

Contributor

phpnode commented Mar 5, 2012

@lightglitch it won't work like that, it works for non ajax pages because the parameters for both grids are in $_GET in the same request, so each grid takes the other grids parameters into account when creating its own URLs. This is not the case with your code unfortunately, and it's pretty hard to solve. I wouldn't get too hung up on it, just make it optional and document this limitation somewhere.

Owner

cebe commented Mar 5, 2012

@phpnode @mdomba I'm afraid github does not allow changing history. This will definitily work if you merged and did not push to GH. But if you pushed you are not able to revert it. (not sure if there is a way, but normally GH will complain)

Contributor

lightglitch commented Mar 5, 2012

@phpnode It does work but make's an AJAX request for every gridview/listview on the page. For example if you have two grids every time you change a page on one of them the other also makes an AJAX request.

This is small side effect that could be improved so that all the grids could be updated with only AJAX request, but for now it's working.

Contributor

phpnode commented Mar 5, 2012

@lightglitch interesting, so with 2 grids, if you paginate one you get 2 ajax requests? that sounds like a bug, what if one of the grids contains a text input that you've started entering text in? that text will get wiped out if you paginate the other grid?

Contributor

lightglitch commented Mar 5, 2012

Not exactly a bug, what happens is:

When you click on a link you add the new URL to the history API.
Both grids are listening to changes in the URL, when that change happens each one updates it self, the result is that each one updates it's links.

Don't forget that each grid only knows about itself on the ajaxUpdate.

Member

mdomba commented Mar 7, 2012

@lightglitch let me know if you are working on adding the option and documentation for it... if not I will work on this.

Contributor

lightglitch commented Mar 7, 2012

@mdomba right now I don't have the time so I appreciate your help on that.

@ghost ghost assigned mdomba Mar 8, 2012

Contributor

lightglitch commented Mar 10, 2012

@mdomba looks I have a little time now, do you started something already or should I?

Member

mdomba commented Mar 11, 2012

@lightglitch feel free to work on this, I had some unexpected errands these last days to work on...

Owner

qiangxue commented Mar 11, 2012

I added issue 482 to keep track of the additional work.

Contributor

steaks commented Mar 11, 2012

Seemed like you guys were busy, so I tried to fix this pull request 490. Hope it helps.

Contributor

phpnode commented Mar 11, 2012

@steaks - for future reference, use the # to make github reference this pull request automatically: #490

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