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

Eager load featured images #498

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Eager load featured images #498

merged 1 commit into from
Apr 20, 2020

Conversation

nizzac
Copy link
Contributor

@nizzac nizzac commented Apr 20, 2020

Loads in featured images in the Posts component loadPosts method

Loads in featured images in the Posts component loadPosts method
@LukeTowers LukeTowers added this to the v1.4.1 milestone Apr 20, 2020
@LukeTowers LukeTowers added Status: Completed Fix is completed and merged into the dev branch; remaining open until it has been merged into master Type: Maintenance Minor maintenance to the code base (i.e. minor bug fixes, styling fixes, translation improvements) labels Apr 20, 2020
@LukeTowers LukeTowers merged commit f85039b into rainlab:master Apr 20, 2020
@bennothommo
Copy link
Contributor

@LukeTowers @nizzac Hmmm, I think this should be optional. We've just effectively added another SQL query to everyone regardless of whether they use the images or not.

@LukeTowers
Copy link
Contributor

@bennothommo I think it's fine given that we already have a potentially unnecessary query for those that don't use categories, and a single query being added on a site that doesn't use featured images (so presumably pretty light weight already) seems like it would outweigh the performance improvement for sites that do utilize featured image as it removes a query for every post displayed on a page at once.

@bennothommo
Copy link
Contributor

@LukeTowers fair, but I'd be more inclined to make the categories optional too, perhaps as component parameters :)

@LukeTowers
Copy link
Contributor

Fair enough @bennothommo, perhaps you could make a PR for it and have it default to eager loading to preserve existing behaviour?

@bennothommo
Copy link
Contributor

@LukeTowers will do :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Fix is completed and merged into the dev branch; remaining open until it has been merged into master Type: Maintenance Minor maintenance to the code base (i.e. minor bug fixes, styling fixes, translation improvements)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants