-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature/3630 #3811
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
Feature/3630 #3811
Conversation
pkarw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't remove the pageSize attribute - rather add the page + pageSize + set the default value of it to the config variable
patzick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's an excellent approach to have the theme config. But we never can assume, that everyone would have this config and same structure, as it's specific for the theme.
So the only use of it should be in the theme folder and never in core files, otherwise it may cause many troubles for implementing projects.
|
@adkamil from my side, please maybe move these theme config settings to the template constants. It's not perfect but theme config is the new feature and we don't want to introduce it now and here. Thanks! |
|
Alrighty! I think this solution meets your approach. |
Related issues
#3630
Short description and why it's useful
I've created config which is used to change features exclusively for specific theme.
At this point it conditions only the number of products loading per page.
Which environment this relates to
Check your case. In case of any doubts please read about Release Cycle
developbranch and want to merge it back todevelopreleasebranch and want to merge it back toreleasehotfixormasterbranch and want to merge it back tohotfixUpgrade Notes and Changelog
No upgrade steps required (100% backward compatibility and no breaking changes)
I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature
I read and followed contribution rules