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

CRM-18286 - Use localStorage to remember datatable page length #8298

Merged
merged 3 commits into from
May 21, 2016

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented May 4, 2016

@colemanw colemanw force-pushed the CRM-18286 branch 3 times, most recently from 3467445 to cf9fd95 Compare May 6, 2016 00:48
// Strip the ids from ajax urls to make pageLength storage more generic
function simplifyUrl(ajax) {
// Datatables ajax prop could be a url string or an object containing the url
var url = typeof ajax === 'object' ? ajax.url : ajax;
Copy link

Choose a reason for hiding this comment

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

I think this could be simplified as:

var url = ajax.url || ajax

@AkA84
Copy link

AkA84 commented May 16, 2016

Successfully replicated the issue on the master branch, on the contact's Activities page and on the Manag Group pages.

On the PR's branch, the issue is gone, the datatable page length being remembered after page refresh on all the major browsers

function simplifyUrl(ajax) {
// Datatables ajax prop could be a url string or an object containing the url
var url = typeof ajax === 'object' ? ajax.url : ajax;
return typeof url === 'string' ? url.replace(/[&?]\w*id=\d+/g, '') : null;
Copy link

Choose a reason for hiding this comment

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

What if you remove the querystring altogether? Is that too generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it would apply to all datatables (at least in WP and Joomla where the civi path is part of the query string).

@totten
Copy link
Member

totten commented May 21, 2016

There was additional feedback from the 4.7.8 spreadsheet, "The patch works as intended, left a couple of suggestions for improving the solution".

If @colemanw and @AkA84 both say it works, then I'm inclined to merge. :)

Will leave it to @colemanw whether to submit follow-up with improvements.

@totten totten merged commit 0749025 into civicrm:master May 21, 2016
@colemanw colemanw deleted the CRM-18286 branch January 30, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants