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

Asset browser: include folders in the paginated results #2928

Closed
wants to merge 5 commits into from
Closed

Asset browser: include folders in the paginated results #2928

wants to merge 5 commits into from

Conversation

arthurperton
Copy link
Contributor

@arthurperton arthurperton commented Nov 27, 2020

This PR could close #2829.

In this solution folders and assets are treated as one collection and the pagination works correctly now. The folders are shown on the first page(s) before all the assets.

If this is accepted as the way to go, the browser Vue component's template could also be simplified a bit, now that folders and assets are treated more uniformly.

Discussion

I would be glad to hear any feedback. Here's a few of my thoughts:

  • Combining the folders and assets is a proper solution to the problem and it has little impact on the existing Vue component for the browser.
  • Right now the pagination specifics are in the Controller, which doesn't feel completely right. Perhaps moving these to a custom QueryBuilder for combined folder/asset results, if possible, would be better.
  • FolderItem uses introspection, which is sometimes considered to be a code smell. I think it is justified / allright here though.

@jasonvarga
Copy link
Member

I feel pretty awful closing this as it's clear that you've put a lot of effort into it.

But unfortunately now since you're not actually paginating the assets (you're getting them all, then slicing and making your own paginator) this becomes very inefficient. If you have many assets, especially if it's somewhere remote like S3, it's going to be doing way too much work.

See #2828

An alternative we discussed was to make folders and assets more visually separated in the UI, and request them separately. Keeping the folders on the page (perhaps collapsible) while you can paginate through assets.

@jasonvarga jasonvarga closed this Dec 4, 2020
@arthurperton
Copy link
Contributor Author

Hi Jason, thanks for your consideration but no worries. I knew the 'risks' taking on a more complex issue like this.

I'd like to respond to your feedback and ask some questions.

  1. While writing the code I was aware that fetching all the results first before slicing them could become a performance issue. It was a red flag at first. So I took a look in the Statamic/Assets/QueryBuilder class to see how this was done more efficiently. Looking at QueryBuilder.getBaseItems() and IteratorBuilder.limitItems() I got the impression that the same thing was happening: gathering all the assets first, then slicing them. So I reckoned the approach was okay after all. What is my error here?

QueryBuilder:

    protected function getBaseItems() {
        ...
        $assets = $this->getContainer()->files($this->folder, $recursive);

        if (empty($this->wheres) && $this->limit) {
            $assets = $assets->skip($this->offset)->take($this->limit)->values();
        }
        ...
    }

IteratorBuilder:

    protected function limitItems($items)
    {
        return $items->slice($this->offset, $this->limit);
    }
  1. On the alternative you discussed. "Keeping the folders on the page (perhaps collapsible) while you can paginate through assets." That's actually what is happening now right, and it led to issue Asset browser showing folders on every paginated result set #2829. Collapsing could improve the UX, but with a lot of subfolders it would stil be awkward I think.

So I still wonder, is there a way to query folders and files together in an efficient way? What actually makes the asset's QueryBuilder efficient right now (see point 1)? Is this something to pursue or would you say upfront it is not possible within the current way QueryBuilders work?

@arthurperton arthurperton deleted the fix/asset-folders-pagination branch December 8, 2020 15:30
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.

Asset browser showing folders on every paginated result set
2 participants