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

Separate module action buttons #9287

Merged
merged 7 commits into from Sep 19, 2018

Conversation

Projects
None yet
8 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Jul 9, 2018

Questions Answers
Branch? develop
Description? The main action of a module is now separated from the secondary ones / The configure link is now a <a> tag, which can be opened in a new tab.
Type? improvement
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? Fixes #10047
How to test? Go to a module page (Manage / Notifications / Updates) and checks there are now 2 buttons for the actions

capture du 2018-07-09 15-53-43


This change is Reviewable

{% set displayAction = action|capitalize|replace({'_': " "})|trans({}, 'Admin.Actions') %}
{% if (action == 'configure') %}

This comment has been minimized.

@matks

matks Jul 12, 2018

Contributor

Maybe we could use a constant here ?
https://twig.symfony.com/doc/2.x/functions/constant.html

@matks

matks Jul 12, 2018

Contributor

Maybe we could use a constant here ?
https://twig.symfony.com/doc/2.x/functions/constant.html

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jul 13, 2018

Member

Sounds to be a good idea. I however do not have defined any action as constant anywhere, this will require a quite huge work.

@Quetzacoalt91

Quetzacoalt91 Jul 13, 2018

Member

Sounds to be a good idea. I however do not have defined any action as constant anywhere, this will require a quite huge work.

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Jul 25, 2018

Contributor

Need rebase for assets

Contributor

PierreRambaud commented Jul 25, 2018

Need rebase for assets

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 17, 2018

Member

Rebased and assets regenerated

Member

Quetzacoalt91 commented Aug 17, 2018

Rebased and assets regenerated

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Aug 17, 2018

@marionf marionf self-assigned this Aug 17, 2018

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 20, 2018

Contributor

@Quetzacoalt91

Even after configuring check & bankwire modules, the configure button is still displayed instead of enable/disable

capture d ecran_179

capture d ecran_180

Contributor

marionf commented Aug 20, 2018

@Quetzacoalt91

Even after configuring check & bankwire modules, the configure button is still displayed instead of enable/disable

capture d ecran_179

capture d ecran_180

@LouiseBonnard

Reviewable status: 0 of 29 files reviewed, 2 unresolved discussions (waiting on @matks and @Quetzacoalt91)


src/PrestaShopBundle/Resources/views/Admin/Module/Includes/action_menu.html.twig, line 48 at r3 (raw file):

Other

Please localize this key in the Admin.Global domains.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 4, 2018

Member

src/PrestaShopBundle/Resources/views/Admin/Module/Includes/action_menu.html.twig, line 48 at r3 (raw file):

Previously, LouiseBonnard wrote…
Other

Please localize this key in the Admin.Global domains.

Sir, yes sir

Member

Quetzacoalt91 commented Sep 4, 2018


src/PrestaShopBundle/Resources/views/Admin/Module/Includes/action_menu.html.twig, line 48 at r3 (raw file):

Previously, LouiseBonnard wrote…
Other

Please localize this key in the Admin.Global domains.

Sir, yes sir

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 4, 2018

Member

@marionf, the issue with the "configure" button is caused by the cache. To avoid loading all the modules on each page, we keep the warning messages in the cache. That's why in some case, you still have this button as a main action.

Member

Quetzacoalt91 commented Sep 4, 2018

@marionf, the issue with the "configure" button is caused by the cache. To avoid loading all the modules on each page, we keep the warning messages in the cache. That's why in some case, you still have this button as a main action.

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 6, 2018

Contributor

@Quetzacoalt91
Sometimes (seems random), when I click on Enable or Disable, the page isn't reloaded and the button don't change
https://drive.google.com/open?id=1N_6Qt5QfIsWwYjNCXYs1TshqV2wZurLg

Contributor

marionf commented Sep 6, 2018

@Quetzacoalt91
Sometimes (seems random), when I click on Enable or Disable, the page isn't reloaded and the button don't change
https://drive.google.com/open?id=1N_6Qt5QfIsWwYjNCXYs1TshqV2wZurLg

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 10, 2018

Member

@marionf I wonder if this is not related to a cached version of the assets of your browser. If the issue occurs again, please refresh with ctrl + alt + R to make sure your have the updated version.

Member

Quetzacoalt91 commented Sep 10, 2018

@marionf I wonder if this is not related to a cached version of the assets of your browser. If the issue occurs again, please refresh with ctrl + alt + R to make sure your have the updated version.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 19, 2018

Member

PR rebased for assets. To be merged once the tests pass.

Member

Quetzacoalt91 commented Sep 19, 2018

PR rebased for assets. To be merged once the tests pass.

@Quetzacoalt91 Quetzacoalt91 merged commit 49e7013 into PrestaShop:develop Sep 19, 2018

1 of 2 checks passed

code-review/reviewable 31 files, 2 discussions left (LouiseBonnard, matks)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 deleted the Quetzacoalt91:separate-module-action-buttons branch Sep 19, 2018

@rdy4ever

This comment has been minimized.

Show comment
Hide comment
@rdy4ever

rdy4ever Sep 28, 2018

Contributor

configuring check & bankwire modules, the configure button is still

I do think that "configure" action should always be the main action, the "disable" action should be under the "other" button. Because you don't really need to disable the module in a new tab, don't you? And, yes, you may still need to reconfigure check and bankwire even after initial configuration. Just my two cents.

Contributor

rdy4ever commented Sep 28, 2018

configuring check & bankwire modules, the configure button is still

I do think that "configure" action should always be the main action, the "disable" action should be under the "other" button. Because you don't really need to disable the module in a new tab, don't you? And, yes, you may still need to reconfigure check and bankwire even after initial configuration. Just my two cents.

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 28, 2018

Contributor

Hello @rdy4ever
We changed this here: #10726

Contributor

marionf commented Sep 28, 2018

Hello @rdy4ever
We changed this here: #10726

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