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

Improved Product pages templates management #8489

Merged
merged 21 commits into from Dec 14, 2017

Conversation

Projects
None yet
8 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Nov 13, 2017

Questions Answers
Branch? develop
Description? Templating of Product Pages are not really manageables, this is an attempt to improve the way templates are used and overriden. This is mainly about re-order and extract sub templates and blocks to make the customization and the maintenance simpler.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? (optional) If this PR fixes a Forge ticket, please add its complete Forge URL.
How to test? The product pages should works as before

I'll try to not make functional BC with 1.7.3.x branch, this will only concerns what I call "Modern modules", the new feature of PrestaShop 1.7.3.

Before

productpages

After

carbon 5


This change is Reviewable

@mickaelandrieu mickaelandrieu changed the title from BO: improved Product pages templates management to Improved Product pages templates management Nov 17, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 17, 2017

Contributor

@kpodemski what do you think? is it enough for module developers needs? /c @firstred

Contributor

mickaelandrieu commented Nov 17, 2017

@kpodemski what do you think? is it enough for module developers needs? /c @firstred

@prestarocket

This comment has been minimized.

Show comment
Hide comment
@prestarocket

prestarocket Nov 17, 2017

Contributor

With your PR, is it possible to add a column in products list just with a hook ? (like hookActionAdminProductsListingFieldsModifier)

Contributor

prestarocket commented Nov 17, 2017

With your PR, is it possible to add a column in products list just with a hook ? (like hookActionAdminProductsListingFieldsModifier)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 17, 2017

Contributor

@prestarocket with my PR you don't need a hook to do that, you only have to override the template products_table.html.twig 👍

See https://gist.github.com/mickaelandrieu/4a02ea05bb1113718aec5432ad418ee5

Contributor

mickaelandrieu commented Nov 17, 2017

@prestarocket with my PR you don't need a hook to do that, you only have to override the template products_table.html.twig 👍

See https://gist.github.com/mickaelandrieu/4a02ea05bb1113718aec5432ad418ee5

@prestarocket

This comment has been minimized.

Show comment
Hide comment
@prestarocket

prestarocket Nov 17, 2017

Contributor

What happens if 2 modules override the same file ?

Contributor

prestarocket commented Nov 17, 2017

What happens if 2 modules override the same file ?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 17, 2017

Contributor

What happens if 2 modules override the same file ?

The latest template in loading will win. Do we need specific hook(s) to manage column(s) and if "yes" what is the best implementation? As you know, hooks cost a lot in performance and we try to avoid them (for now).

Contributor

mickaelandrieu commented Nov 17, 2017

What happens if 2 modules override the same file ?

The latest template in loading will win. Do we need specific hook(s) to manage column(s) and if "yes" what is the best implementation? As you know, hooks cost a lot in performance and we try to avoid them (for now).

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Nov 20, 2017

Contributor

:lgtm:


Reviewed 47 of 56 files at r1, 2 of 3 files at r2, 9 of 16 files at r3, 1 of 4 files at r5, 14 of 14 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Contributor

LittleBigDev commented Nov 20, 2017

:lgtm:


Reviewed 47 of 56 files at r1, 2 of 3 files at r2, 9 of 16 files at r3, 1 of 4 files at r5, 14 of 14 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 23, 2017

Member

@mickaelandrieu, it seems we need a rebase here after #8530

Member

Quetzacoalt91 commented Nov 23, 2017

@mickaelandrieu, it seems we need a rebase here after #8530

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Nov 28, 2017

Contributor

@Quetzacoalt91 it's rebased !

Contributor

mickaelandrieu commented Nov 28, 2017

@Quetzacoalt91 it's rebased !

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Nov 28, 2017

Contributor

Reviewed 1 of 56 files at r1, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Contributor

LittleBigDev commented Nov 28, 2017

Reviewed 1 of 56 files at r1, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@mickaelandrieu mickaelandrieu referenced this pull request Dec 6, 2017

Merged

Migrate Logs page to Symfony #8523

19 of 19 tasks complete

mickaelandrieu added a commit to mickaelandrieu/PrestaShop that referenced this pull request Dec 11, 2017

mickaelandrieu added a commit to mickaelandrieu/PrestaShop that referenced this pull request Dec 12, 2017

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Dec 13, 2017

Contributor

@Quetzacoalt91 can you please review this one? I'd like to see it verified by QA asap

Contributor

mickaelandrieu commented Dec 13, 2017

@Quetzacoalt91 can you please review this one? I'd like to see it verified by QA asap

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Dec 14, 2017

Contributor

@marionf

The image editing block is empty

The error is also present in develop branch => not related

Labels are missing for delivery options

The error is also present in develop branch => not related

When I check a supplier I can't save And the form to add references isn't displayed (this one)

True story, I'll fix theses issues. I've rebased from develop, these last 2 issues are fixed, you may need to install assets for both "default" and "new-theme" themes

Contributor

mickaelandrieu commented Dec 14, 2017

@marionf

The image editing block is empty

The error is also present in develop branch => not related

Labels are missing for delivery options

The error is also present in develop branch => not related

When I check a supplier I can't save And the form to add references isn't displayed (this one)

True story, I'll fix theses issues. I've rebased from develop, these last 2 issues are fixed, you may need to install assets for both "default" and "new-theme" themes

@Quetzacoalt91 Quetzacoalt91 merged commit 26e9e56 into PrestaShop:develop Dec 14, 2017

2 of 3 checks passed

code-review/reviewable 1 file left (LittleBigDev)
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:bogoss/refactor-product-page branch Dec 14, 2017

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