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

Refactor ps_linklist module using 1.7 improvements #50

Merged
merged 47 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@jolelievre
Contributor

jolelievre commented Nov 9, 2018

Questions Answers
Branch? master
Description? Symfony migration of the module using the new Grid component, namespaces, Symfony controllers, ...
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test?

This change is Reviewable

@PierreRambaud

Go to major version please

jolelievre added some commits Nov 9, 2018

- 'new-products'
- 'best-sales'
prestashop.module.link_block.choice_provider.static_page:

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 9, 2018

Contributor

how about link_block.choice_provider.static_page ? shorter

/**
* Class LegacyBlockCache.
*/
final class LegacyLinkBlockCache implements LinkBlockCacheInterface

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 9, 2018

Contributor

I'm not sure we need an interface here: are people supposed to be able to change this class?

/**
* Class LinkBlockFormDataProvider.
*/
class LinkBlockFormDataProvider implements FormDataProviderInterface

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 9, 2018

Contributor

"By the book" the validation should be done in FormHandlerInterface, this class have too many responsibilities.

I don't say you have to update it 👍

{
$builder
->add('title', TextType::class, [
'label' => $this->trans('Title', 'Modules.Linklist.Admin'),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 9, 2018

Contributor

you're so lucky to be able to use it, we need to unlock translation system for external modules :(

* International Registered Trademark & Property of PrestaShop SA
*#}
{%- block choice_widget_expanded -%}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 9, 2018

Contributor

not sure it's specific to your needs: shouldn't be part of core that needs to be improved?

This comment has been minimized.

@jolelievre

jolelievre Nov 14, 2018

Contributor

probably, I will hint that to sarunas

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Nov 9, 2018

@PierreRambaud I did update the version to 3.0.0, I don't understand what you want
@mickaelandrieu are you serious? 🤪 you make a feedback now after I finally got to merge this PR after 2 months and a half? 😅

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 10, 2018

@jolelievre I wrote this comment before you did it :D

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 10, 2018

@mickaelandrieu he's right, the previous PR was in review for 2 months :/

@jolelievre jolelievre merged commit 9bf37ec into master Nov 14, 2018

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