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

Introduce switch type in forms #8705

Merged
merged 12 commits into from Mar 28, 2018

Conversation

Projects
None yet
4 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Jan 26, 2018

Questions Answers
Branch? develop
Description? Add the switch representation on our new forms (ON / OFF)
Type? new feature
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? Our migrated pages must use the switch instead of a select for On/Off cases

capture du 2018-01-26 11-51-57


This change is Reviewable

'show_notifs_new_orders' => $this->configuration->get('PS_SHOW_NEW_ORDERS'),
'show_notifs_new_customers' => $this->configuration->get('PS_SHOW_NEW_CUSTOMERS'),
'show_notifs_new_messages' => $this->configuration->get('PS_SHOW_NEW_MESSAGES'),
'show_notifs_new_orders' => (bool)$this->configuration->get('PS_SHOW_NEW_ORDERS'),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jan 27, 2018

Contributor

we shouldn't have to cast values, if its not boolean values lets change them in database.

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

I had to, as the Configuration table allows every primitive types to be stored.

$this->configuration->set('PS_SMARTY_CACHE', $configuration['cache']);
$this->configuration->set('PS_SMARTY_LOCAL', $configuration['multi_front_optimization']);
$this->configuration->set('PS_SMARTY_CACHE', (bool)$configuration['cache']);
$this->configuration->set('PS_SMARTY_LOCAL', (bool)$configuration['multi_front_optimization']);

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jan 27, 2018

Contributor

same here, I'm not easy with new PHP code fixing possible database issues! How about a $configuration->getBoolean('KEY') instead?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

Ah, realy nice suggestion here!

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 29, 2018

Member

We could reuse the same features as the ParameterBag from Symfony, but as an interface with the database. https://api.symfony.com/3.0/Symfony/Component/HttpFoundation/ParameterBag.html

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Feb 2, 2018

Contributor

agree :)

{% spaceless %}
<span class="ps-switch">
{% for choice in choices %}
{% set name = id ~'_' ~ choice.value %}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jan 27, 2018

Contributor
- name
+ input_id
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Feb 20, 2018

@eternoendless @mickaelandrieu can you have a look at the commit 2d96cd5?

Thanks

@eternoendless

This comment has been minimized.

Member

eternoendless commented Feb 20, 2018

Reviewed 14 of 15 files at r1, 1 of 1 files at r2, 10 of 13 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/Adapter/Configuration.php, line 43 at r4 (raw file):

    {
        // Do nothing
        if (count($parameters)) {

I think that it's generally better to use !empty() instead of count() when you are trying to know if an array is empty. Not only because it makes what you're testing for clearer, but there's also a performance difference (albeit tiny) in favor of empty since it's a language construct, whereas count instead of a function call.


src/PrestaShopBundle/Form/Admin/Type/SwitchType.php, line 51 at r4 (raw file):

            'choices' => array(
                'No' => false,
                'Yes' => true,

Are these values translated automagically?


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Feb 20, 2018

src/PrestaShopBundle/Form/Admin/Type/SwitchType.php, line 51 at r4 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Are these values translated automagically?

Yes they are, look at the "choice_translation_domain" key :)


Comments from Reviewable

@eternoendless

This comment has been minimized.

Member

eternoendless commented Feb 20, 2018

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Member

eternoendless commented Feb 20, 2018

Code looks good but travis build is failing


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Feb 21, 2018

I had to remove all occurrences of bootstrap.php.cache in order to make the tests pass. Really weird.

@eternoendless

This comment has been minimized.

Member

eternoendless commented Feb 21, 2018

Weird indeed, why did we have that in the first place?


Reviewed 1 of 1 files at r5, 7 of 7 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Feb 21, 2018

is the file still generated with php 5.6? it contains inline definitions of some classes/services to speed up the boot of Symfony application (not specific to PrestaShop). It's useless in PHP7+, because others optimizations are done in this case

@marionf

This comment has been minimized.

Contributor

marionf commented Mar 13, 2018

@Quetzacoalt91

I don't see the swith on the maintenance page
capture du 2018-03-13 16-26-23

And some "No" are in grey and others in white. If I click on the grey one, it becomes white.
Can we have all in the same color ?
capture du 2018-03-13 16-27-02

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Mar 13, 2018

That's because the maintenance page PR was merged after this one was created. :)

I will apply the change on this one as well.

@marionf

This comment has been minimized.

Contributor

marionf commented Mar 27, 2018

@Quetzacoalt91

Some buttons are ok but others are still grey
capture du 2018-03-27 10-23-24

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Mar 27, 2018

Sorry @marionf, I missed the other screenshot. The values were not boolean, that why you had this visual issue

@marionf

This comment has been minimized.

Contributor

marionf commented Mar 28, 2018

@Quetzacoalt91 Can you solve it also on the maintenance page ?

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Mar 28, 2018

Done, thanks :)

@marionf

This comment has been minimized.

Contributor

marionf commented Mar 28, 2018

Thank you @Quetzacoalt91 👍

@marionf marionf added QA ✔️ and removed waiting for QA labels Mar 28, 2018

@mickaelandrieu mickaelandrieu merged commit 62d2d33 into PrestaShop:develop Mar 28, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
code-review/reviewable 16 files left (eternoendless)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Mar 28, 2018

Thanks everyone!

@eternoendless eternoendless added this to the 1.7.4.0 milestone Apr 13, 2018

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