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

Pagination Bugfixes #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Pagination Bugfixes #17

wants to merge 3 commits into from

Conversation

NightJar
Copy link

@NightJar NightJar commented Nov 7, 2011

Fixed issue where pagination stopped working after first load
Fixed issue where items could not be removed if they were not on the first page

ajshort added a commit that referenced this pull request Nov 7, 2011
@ajshort
Copy link
Owner

ajshort commented Nov 7, 2011

I've fixed the issue with items not being removable if not on the first page - I used a different fix so we don't have to load the entire list just to get one item.

Could you give some more detail on the pagination issue? I can't seem to break it by performing searches etc.

@NightJar
Copy link
Author

NightJar commented Nov 7, 2011

I found when loading a page with a ManyManyPickerField that I could browse back and forward through the existing relations fine. However subsequent calls, eg. after saving (or reloading the page by clicking in the SiteTree), clicking 'next' does nothing. It loads, and the correct data is returned in the ajax call, however the field on page does not get updated.

I fixed the issue by giving a fieldset parent to the ID selector you used to replace the field.

I assume that jQuery was replacing the out of document (the freshly returned) field instead of the one on page (so the field basicaly replaced itself off screen), due to there being 2 elements with the same ID tag.
It's a simple one line fix in the ItemSetField.js file (though my patch is a few lines with comments and keeping the bracket format consistant).
You can see the change by itself in my original (closed) pull request, which I closed because I thought my fix and the remove issue were related.

@NightJar
Copy link
Author

NightJar commented Nov 7, 2011

Hmm, might not have been the relations, but rather the search form results (to add a relation). Sorry, memory a bit foggy after the weekend break.

@ajshort
Copy link
Owner

ajshort commented Nov 7, 2011

I think you might have encountered the same issue as #13 - can you see if this is fixed?

@NightJar
Copy link
Author

NightJar commented Nov 8, 2011

Issue #13 prevented a user from saving relations that weren't on the current paginated page at the time of saving. This is in the Search dialog when trying to add a relation (pre-save), the pagination 'next' will not work after the first load.

I found this bug after reading #13 before it was closed and commenting out the offending line on my local dev copy.

To replicate: Load a page with an ItemPickerField, search to add a new relation. Click next to confirm that pagination works. Close dialog (either with X or by picking a relation, doesn't matter). Click the same page (or any ItemPickerField page) in the site tree again (don't use full page refresh). Wait for page to reload, again open the ItemPickerField search dialog, click next. Results page will not update.

Of course, you must have test data that exceeds one search page in length. Like 20 (for the default of 15 per page).

@ajshort
Copy link
Owner

ajshort commented Nov 8, 2011

Which browser are you using? This is working fine for me.

@NightJar
Copy link
Author

NightJar commented Nov 8, 2011

Strange. Firefox.

Just tested it on the windows box too. Doesn't work there either in 7.0.1
(original test machine Linux - FFv 3.6.23)


Also just tested on windows:

Chrome 14.0.835.202 m - issue exists
IE 9.0.8112.16421 - issue exists
Opera 11.51.1087 - issue exists
Opera 11.52.1100 - issue exists
Firefox 8.0 - issue exists
Chrome 15.0.874.106 m - issue exists


You need to have the (sitetree, not web) page reload through ajax before the issue comes up. Be that through navigating in the sitetree or by saving. If you click "Pages" or the refresh button the issue will not occur.

@ajshort
Copy link
Owner

ajshort commented Nov 9, 2011

Thanks for the detailed info - I managed to track it down - when you close the dialog the first time it's just hidden, not removed, and the search results are loaded into this rather than the visible one. I'll work on a fix.

@NightJar
Copy link
Author

NightJar commented Nov 9, 2011

That's what my patch fixes ;)

Well, this commit specifically:
https://github.com/NightJar/silverstripe-itemsetfield/commit/8ff3b1a5c99b4a0bbcaea2f711f53d5614f6fcbc

Simply prepending "fieldset" to the jQuery selector seems to do the trick (after checking that we have a results list first).

But there maybe a better way. I kinda hacked it in a hurry.

@ajshort
Copy link
Owner

ajshort commented Nov 9, 2011

I know - but I think it would be better to fix it by removing unnecessary dialogs from the DOM rather than working around it.

@NightJar
Copy link
Author

NightJar commented Nov 9, 2011

True that :)

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.

None yet

2 participants