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

Only lazy load categories dropdown if there is a large number of categories #637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

purplespider
Copy link
Contributor

By lazy loading the dropdown, authors cannot see the list of existing categories to pick from and have to think of one from memory and start typing it.

This only sets the dropdown values to lazy load if there are more than 15 existing categories. 15 was chosen as this is also the number of categories that appear on one page of the Categories grid field.

@purplespider
Copy link
Contributor Author

Is this good to be merged?

Copy link
Contributor

@dhensby dhensby left a comment

Choose a reason for hiding this comment

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

Stylistic change for you.

Also, why 15? That doesn't seem like a particularly large number for the application layer to handle/load. Even 100 should be reasonably safe to handle. What's the default pagination on a GridField, maybe that's a sensible number - alternatively, it could be configurable.

Comment on lines +315 to +318
$shouldLazyLoadCategories = true;
if($this->Categories()->count() < 15) {
$shouldLazyLoadCategories = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be changed to be a bit more readable (IMO) and also a bit better to phrase it in the positive sense "lazy load if more than 15" rather than currently "don't lazy load if less than 15".

Suggested change
$shouldLazyLoadCategories = true;
if($this->Categories()->count() < 15) {
$shouldLazyLoadCategories = false;
}
$shouldLazyLoadCategories = $this->Categories()->count() > 15;

@purplespider
Copy link
Contributor Author

Also, why 15?

Personally, I would have just set lazy loading to always be false for categories. I'd imagine that only in edge cases would there be so many as to cause an issue and it could be easily overridden in an extension then.

However, I assumed it was done for a reason, and as mentioned I just picked 15 as this is also the number of categories that appear on one page of the Categories grid field, which, I've just checked, is actually the $default_items_per_page for GridFieldPaginator.

@purplespider
Copy link
Contributor Author

Thinking about it, it's not just about technical load, but also UX. Showing a dropdown list of 100 items to a user isn't very useful, it's unlikely they'll manually look through the whole list, and would just type to filter anyway.

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.

2 participants