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

Link modules routes, position column and extension, GridPositionUpdater #10414

Merged
merged 17 commits into from Sep 20, 2018

Conversation

@jolelievre
Contributor

jolelievre commented Sep 13, 2018

Questions Answers
Branch? develop
Description? Manage _legacy_link route parameter to allow matching modules routes, add PositionColumn in Grid Component, add PositionExtension javascript, add GridPositionUpdater service which allows to update row positions
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9746
How to test? Please indicate how to best verify that this PR is correct.

This change is Reviewable

@jolelievre

This comment has been minimized.

Show comment
Hide comment
@jolelievre
Contributor

jolelievre commented Sep 13, 2018

Show outdated Hide outdated classes/Link.php Outdated
Show outdated Hide outdated classes/Link.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/DoctrineGridPositionUpdater.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/DoctrineGridPositionUpdater.php Outdated
Show outdated Hide outdated src/Core/Grid/Column/Type/Catalog/PositionColumn.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/DoctrineGridPositionUpdater.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/GridPositionUpdaterInterface.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/GridPositionUpdaterInterface.php Outdated
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 13, 2018

Contributor

remove extra lines

@mickaelandrieu

mickaelandrieu Sep 13, 2018

Contributor

remove extra lines

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

still needs to be done

@mickaelandrieu

mickaelandrieu Sep 19, 2018

Contributor

still needs to be done

Show outdated Hide outdated src/Core/Grid/Position/RowUpdateInterface.php Outdated
Show outdated Hide outdated src/PrestaShopBundle/Controller/Admin/FrameworkBundleAdminController.php Outdated
Show outdated Hide outdated src/PrestaShopBundle/EventListener/ModuleActivatedListener.php Outdated
Show resolved Hide resolved src/PrestaShopBundle/EventListener/ModuleActivatedListener.php Outdated
Show resolved Hide resolved src/PrestaShopBundle/EventListener/ModuleActivatedListener.php
@mickaelandrieu

and my review is done! Good job @jolelievre, some changes expected 👍

Show outdated Hide outdated src/PrestaShopBundle/Security/Annotation/ModuleActivated.php Outdated
@@ -94,6 +94,10 @@
col-sm-10
{%- endblock form_group_class %}
{% block form_row_class -%}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 13, 2018

Contributor

why do you need to do that in this pull request?

@mickaelandrieu

mickaelandrieu Sep 13, 2018

Contributor

why do you need to do that in this pull request?

This comment has been minimized.

@jolelievre

jolelievre Sep 13, 2018

Contributor

so that I can override it in my module, I originally even wanted to change default class with form-group row but I don't want to risk breaking the other pages

@jolelievre

jolelievre Sep 13, 2018

Contributor

so that I can override it in my module, I originally even wanted to change default class with form-group row but I don't want to risk breaking the other pages

@sarjon

although i like the idea of Position extension for grid, but should it be part of 1.7.5? i wish we could have more time to test it on at least few different grids to see if its good enough? wdyt @mickaelandrieu ?

Show outdated Hide outdated ...mes/new-theme/js/components/grid/extension/catalog/position-extension.js Outdated
Show outdated Hide outdated ...mes/new-theme/js/components/grid/extension/catalog/position-extension.js Outdated
Show outdated Hide outdated admin-dev/themes/new-theme/js/components/translatable-input.js Outdated
Show outdated Hide outdated src/Core/Grid/Column/Type/Catalog/PositionColumn.php Outdated
Show outdated Hide outdated src/Core/Grid/Column/Type/Catalog/PositionColumn.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/GridPositionUpdater.php Outdated
Show resolved Hide resolved src/Core/Grid/Position/GridPositionUpdater.php
Show outdated Hide outdated src/Core/Grid/Position/RowUpdateInterface.php Outdated
Show outdated Hide outdated src/Core/Grid/Position/UpdateHandler/DoctrinePositionUpdateHandler.php Outdated
* Class PositionUpdate contains the modifications needed
* to update the grid positions.
*/
final class PositionUpdate implements PositionUpdateInterface

This comment has been minimized.

@sarjon

sarjon Sep 13, 2018

Member

ModifiedPositionInformation as a name? so it is noun instead of verb, wdyt?

@sarjon

sarjon Sep 13, 2018

Member

ModifiedPositionInformation as a name? so it is noun instead of verb, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Sep 16, 2018

Contributor

modified to PositionModificationInterface

@jolelievre

jolelievre Sep 16, 2018

Contributor

modified to PositionModificationInterface

@matks matks added the migration label Sep 14, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Sep 16, 2018

Contributor

although i like the idea of Position extension for grid, but should it be part of 1.7.5? i wish we could have more time to test it on at least few different grids to see if its good enough? wdyt @mickaelandrieu ?

I'm in favor of accepting it, and with the right interfaces we can change the implementation if needed... we could mark the current implementation as @internal /c @eternoendless.

Contributor

mickaelandrieu commented Sep 16, 2018

although i like the idea of Position extension for grid, but should it be part of 1.7.5? i wish we could have more time to test it on at least few different grids to see if its good enough? wdyt @mickaelandrieu ?

I'm in favor of accepting it, and with the right interfaces we can change the implementation if needed... we could mark the current implementation as @internal /c @eternoendless.

@mickaelandrieu

Some minor comments to be addressed, good job @jolelievre

@mickaelandrieu

add an assertion in GridPositionUpdaterTest::testUpdate

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

quick fix needed at
PrestaShop/ps_linklist#48
thanks @jolelievre

Contributor

mbadrani commented Sep 20, 2018

quick fix needed at
PrestaShop/ps_linklist#48
thanks @jolelievre

@mbadrani mbadrani added Bug and removed waiting for QA labels Sep 20, 2018

@PierreRambaud PierreRambaud added waiting for QA and removed Bug labels Sep 20, 2018

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

So Good !

Contributor

mbadrani commented Sep 20, 2018

So Good !

@mbadrani mbadrani added QA ✔️ and removed waiting for QA labels Sep 20, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit d588c64 into PrestaShop:develop Sep 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 20, 2018

Member

Thank you all!

Member

Quetzacoalt91 commented Sep 20, 2018

Thank you all!

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