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

Sites manager first last buttons #12276

Closed
wants to merge 5 commits into
base: 3.x-dev
from

Conversation

Projects
None yet
5 participants
@iMarkus
Copy link
Contributor

iMarkus commented Nov 15, 2017

Adds First and Last buttons to SiteManager

Maybe this code is not the best way to get all sites count:

function lastPage()
{
    sitesManagerAPI.getSitesIdWithAdminAccess(function (siteIds) {
        if (siteIds && siteIds.length) {
            setCurrentPage(Math.floor(siteIds.length / model.pageSize));
            fetchLimitedSitesWithAdminAccess();
        }
    });
}

grafik

iMarkus added some commits Nov 15, 2017

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Nov 22, 2017

Thanks for the useful pull request @iMarkus we will review it soon.

@mattab mattab added this to the 3.2.2 milestone Nov 22, 2017

@mattab mattab modified the milestones: 3.5.0, 3.4.1 Mar 26, 2018

@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Mar 26, 2018

seems the travis tests aren't triggered anymore for this PR. I'll try closing and reopening...

@sgiehl sgiehl closed this Mar 26, 2018

@sgiehl sgiehl reopened this Mar 26, 2018

@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Mar 26, 2018

@iMarkus What was the intention of adding the additional offset params? Speeding up the requests as it does not need to query more sites than needed?

@iMarkus

This comment has been minimized.

Copy link
Contributor

iMarkus commented Mar 27, 2018

@sgiehl Exactly

{
$sitesId = $this->getSitesIdWithAdminAccess();
if ($pattern === false) {
$sites = $this->getSitesFromIds($sitesId, $limit);
$sites = $this->getSitesFromIds($sitesId, $limit, $offset);
} else {
$sites = $this->getModel()->getPatternMatchSites($sitesId, $pattern, $limit);

This comment has been minimized.

@tsteur

tsteur Apr 23, 2018

Member

The offset would need to be applied here as well?

This comment has been minimized.

@tsteur

tsteur Apr 23, 2018

Member

I mean in getPatternMatchSites

@@ -63,6 +75,16 @@
fetchLimitedSitesWithAdminAccess();
}

function lastPage()
{
sitesManagerAPI.getSitesIdWithAdminAccess(function (siteIds) {

This comment has been minimized.

@tsteur

tsteur Apr 23, 2018

Member

I haven't tested it, but I reckon it might not work when the user is currently searching for a site and then goes to the last page because I think here getSitesIdWithAdminAccess would return really all sites whereas the number of results be lower when searching.

This comment has been minimized.

@iMarkus

iMarkus Apr 25, 2018

Contributor

Hi, yes you are right.

@mattab mattab modified the milestones: 3.5.0, 3.6.0 May 2, 2018

@mattab mattab added Pull Request WIP and removed Needs Review labels May 2, 2018

@sgiehl

This comment has been minimized.

Copy link
Member

sgiehl commented Jul 16, 2018

@iMarkus will you be able to have a look at the comments soon? We would like to integrate this changes in our next release if possible

@diosmosis diosmosis modified the milestones: 3.6.0, 3.7.0 Jul 24, 2018

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Dec 19, 2018

@iMarkus not sure if you're still onto this. I presume it might not be needed anymore for you? If you could look at the comments that would be great. Otherwise we might instead just close the PR and create an issue for it so this awesome suggestion doesn't get lost 👍

@mattab mattab removed this from the 3.8.0 milestone Dec 31, 2018

@iMarkus

This comment has been minimized.

Copy link
Contributor

iMarkus commented Jan 8, 2019

@tsteur We can close it and maybe sometime the feature will come :) I am not onto this anymore.

@iMarkus iMarkus closed this Jan 8, 2019

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