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

Product Page - Shipping: delivery times #8419

Merged
merged 9 commits into from Oct 30, 2017

Conversation

@kompilorb
Contributor

kompilorb commented Oct 16, 2017

Questions Answers
Branch? develop
Description? Product Page - Shipping: delivery times
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/FF-96
How to test? Use new feature for administrate shipping by product. See result on Front Office, on the product.

This change is Reviewable

@kompilorb kompilorb changed the title from 1.7.3 productpage to Product Page - Shipping: delivery times Oct 16, 2017

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.


admin-dev/themes/new-theme/scss/pages/_product_page.scss, line 1000 at r1 (raw file):

    }
  }
}

Missing a single blank line at the end of file


classes/Product.php, line 259 at r1 (raw file):

    public $additional_delivery_times = 1;

    /** @var string delivery in-stock information */

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery in-stock information (short definition)
  * 
  * Regular delivery time (for in stock products) (longer description to explain better)
  * 
  * @var string
  * /

classes/Product.php, line 263 at r1 (raw file):


    /** @var string delivery out-stock information */
    public $delivery_out_stock;

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery out-stock information (short definition)
  * 
  * Delivery time for out of stock products (because more delays to replenish stock etc etc.)
  * Only for allowed orders. (longer description to explain better, multiple lines are possible)
  * 
  * @var string
  * /

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 136 at r1 (raw file):

        ->add('delivery_out_stock', 'Symfony\Component\Form\Extension\Core\Type\TextType', array(
            'required' => false,
            'label' => $this->translator->trans('Delivery time of out-of-stock products with allowed orders:', [], 'Admin.Catalog.Feature'),

Try stay within the PSR-2 line length soft limit (120 characters).
There is no hard limit though, and you can keep it as-is if you consider it will be easier to read and understand.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.


admin-dev/themes/new-theme/scss/pages/_product_page.scss, line 1000 at r1 (raw file):

    }
  }
}

Missing a single blank line at the end of file


classes/Product.php, line 259 at r1 (raw file):

    public $additional_delivery_times = 1;

    /** @var string delivery in-stock information */

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery in-stock information (short definition)
  * 
  * Regular delivery time (for in stock products) (longer description to explain better)
  * 
  * @var string
  * /

classes/Product.php, line 263 at r1 (raw file):


    /** @var string delivery out-stock information */
    public $delivery_out_stock;

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery out-stock information (short definition)
  * 
  * Delivery time for out of stock products (because more delays to replenish stock etc etc.)
  * Only for allowed orders. (longer description to explain better, multiple lines are possible)
  * 
  * @var string
  * /

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 136 at r1 (raw file):

        ->add('delivery_out_stock', 'Symfony\Component\Form\Extension\Core\Type\TextType', array(
            'required' => false,
            'label' => $this->translator->trans('Delivery time of out-of-stock products with allowed orders:', [], 'Admin.Catalog.Feature'),

Try stay within the PSR-2 line length soft limit (120 characters).
There is no hard limit though, and you can keep it as-is if you consider it will be easier to read and understand.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@LittleBigDev

Some changes are requested. See the Reviewable comments above.

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 263 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery out-stock information (short definition)
  * 
  * Delivery time for out of stock products (because more delays to replenish stock etc etc.)
  * Only for allowed orders. (longer description to explain better, multiple lines are possible)
  * 
  * @var string
  * /

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 263 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery out-stock information (short definition)
  * 
  * Delivery time for out of stock products (because more delays to replenish stock etc etc.)
  * Only for allowed orders. (longer description to explain better, multiple lines are possible)
  * 
  * @var string
  * /

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 259 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery in-stock information (short definition)
  * 
  * Regular delivery time (for in stock products) (longer description to explain better)
  * 
  * @var string
  * /

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 259 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Please be more verbose about this property.
You can use the multiline docblock for this :

/**
  * Delivery in-stock information (short definition)
  * 
  * Regular delivery time (for in stock products) (longer description to explain better)
  * 
  * @var string
  * /

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

admin-dev/themes/new-theme/scss/pages/_product_page.scss, line 1000 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Missing a single blank line at the end of file

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

admin-dev/themes/new-theme/scss/pages/_product_page.scss, line 1000 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Missing a single blank line at the end of file

Done.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 17, 2017

Member

Reviewed 4 of 9 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


classes/Product.php, line 259 at r2 (raw file):

     * Type of delivery time
     *
     * Choose witch parameters use for give information delivery.

Small typo here: "witch" --> "which" (otherwise we'd be talking about a mean old lady who performs magic and travels on a broomstick 😉 )


classes/Product.php, line 262 at r2 (raw file):

     * 0 - none
     * 1 - use default information
     * 2 - use product information

Can we use constants for these values?


install-dev/upgrade/sql/1.7.3.0.sql, line 14 at r2 (raw file):

AFTER delivery_times

There's s a typo here, should be additional_delivery_times instead


install-dev/upgrade/sql/1.7.3.0.sql, line 15 at r2 (raw file):

ALTER TABLE `PREFIX_product` ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`;
ALTER TABLE `PREFIX_product` ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `delivery_times`;
ALTER TABLE `PREFIX_product` ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`;

FYI, you could merge your ALTERs:

ALTER TABLE `PREFIX_product`
    ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`,
    ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `additional_delivery_times`
    ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`

*Note that the first column to add must last


src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 123 at r2 (raw file):

        ->add('additional_delivery_times', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', array(
            'choices' =>  array(
                '0' => $this->translator->trans('None', [], 'Admin.Catalog.Feature'),

We need to use the long array syntax until 1.7.4, please change it everywhere 🙂


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

No need for onclick=""


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

What does the sensitive class do? 🤔


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

Wouldn't it be simpler to extract the <a> and the <i> from the translation? I don't see any need to keep it as part of the translation itself.

<a href="{{ getAdminLink("AdminPPreferences") }}" class="btn sensitive px-0"><i class="material-icons">open_in_new</i>
{{ "edit"|trans({}, 'Admin.Catalog.Help') }}</a>

themes/classic/templates/catalog/_partials/product-prices.tpl, line 103 at r2 (raw file):

        {/if}
      {/if}
      {if $product.additional_delivery_times == 2}

Shouldn't this be {elseif ...}?


Comments from Reviewable

Member

eternoendless commented Oct 17, 2017

Reviewed 4 of 9 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


classes/Product.php, line 259 at r2 (raw file):

     * Type of delivery time
     *
     * Choose witch parameters use for give information delivery.

Small typo here: "witch" --> "which" (otherwise we'd be talking about a mean old lady who performs magic and travels on a broomstick 😉 )


classes/Product.php, line 262 at r2 (raw file):

     * 0 - none
     * 1 - use default information
     * 2 - use product information

Can we use constants for these values?


install-dev/upgrade/sql/1.7.3.0.sql, line 14 at r2 (raw file):

AFTER delivery_times

There's s a typo here, should be additional_delivery_times instead


install-dev/upgrade/sql/1.7.3.0.sql, line 15 at r2 (raw file):

ALTER TABLE `PREFIX_product` ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`;
ALTER TABLE `PREFIX_product` ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `delivery_times`;
ALTER TABLE `PREFIX_product` ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`;

FYI, you could merge your ALTERs:

ALTER TABLE `PREFIX_product`
    ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`,
    ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `additional_delivery_times`
    ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`

*Note that the first column to add must last


src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 123 at r2 (raw file):

        ->add('additional_delivery_times', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', array(
            'choices' =>  array(
                '0' => $this->translator->trans('None', [], 'Admin.Catalog.Feature'),

We need to use the long array syntax until 1.7.4, please change it everywhere 🙂


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

No need for onclick=""


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

What does the sensitive class do? 🤔


src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

                {{ form_widget(child) }}
                <span class="small font-secondary">
                  {{ "[1][2]edit[/1]"|trans({}, 'Admin.Catalog.Help')|replace({'[1]': '<a href="' ~ getAdminLink("AdminPPreferences") ~'" onclick="" class="btn sensitive px-0">', '[/1]': '</a>', '[2]': '<i class="material-icons">open_in_new</i>'})|raw }}

Wouldn't it be simpler to extract the <a> and the <i> from the translation? I don't see any need to keep it as part of the translation itself.

<a href="{{ getAdminLink("AdminPPreferences") }}" class="btn sensitive px-0"><i class="material-icons">open_in_new</i>
{{ "edit"|trans({}, 'Admin.Catalog.Help') }}</a>

themes/classic/templates/catalog/_partials/product-prices.tpl, line 103 at r2 (raw file):

        {/if}
      {/if}
      {if $product.additional_delivery_times == 2}

Shouldn't this be {elseif ...}?


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

install-dev/upgrade/sql/1.7.3.0.sql, line 15 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

FYI, you could merge your ALTERs:

ALTER TABLE `PREFIX_product`
    ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`,
    ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `additional_delivery_times`
    ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`

*Note that the first column to add must last

I know but i have take the old structure view in sql file (not merge the alter)


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

install-dev/upgrade/sql/1.7.3.0.sql, line 15 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

FYI, you could merge your ALTERs:

ALTER TABLE `PREFIX_product`
    ADD `additional_delivery_times` tinyint(1) unsigned NOT NULL DEFAULT '1' AFTER `out_of_stock`,
    ADD `delivery_in_stock` varchar(255) DEFAULT NULL AFTER `additional_delivery_times`
    ADD `delivery_out_stock` varchar(255) DEFAULT NULL AFTER `delivery_in_stock`

*Note that the first column to add must last

I know but i have take the old structure view in sql file (not merge the alter)


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 136 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Try stay within the PSR-2 line length soft limit (120 characters).
There is no hard limit though, and you can keep it as-is if you consider it will be easier to read and understand.

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 136 at r1 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Try stay within the PSR-2 line length soft limit (120 characters).
There is no hard limit though, and you can keep it as-is if you consider it will be easier to read and understand.

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What does the sensitive class do? 🤔

I take the same class css from the same button. I'm not sure what he does ?


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What does the sensitive class do? 🤔

I take the same class css from the same button. I'm not sure what he does ?


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 262 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Can we use constants for these values?

Not at this moment


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 262 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Can we use constants for these values?

Not at this moment


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

themes/classic/templates/catalog/_partials/product-prices.tpl, line 103 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Shouldn't this be {elseif ...}?

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

themes/classic/templates/catalog/_partials/product-prices.tpl, line 103 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Shouldn't this be {elseif ...}?

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 259 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Small typo here: "witch" --> "which" (otherwise we'd be talking about a mean old lady who performs magic and travels on a broomstick 😉 )

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 259 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Small typo here: "witch" --> "which" (otherwise we'd be talking about a mean old lady who performs magic and travels on a broomstick 😉 )

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

install-dev/upgrade/sql/1.7.3.0.sql, line 14 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

AFTER delivery_times

There's s a typo here, should be additional_delivery_times instead

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

install-dev/upgrade/sql/1.7.3.0.sql, line 14 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

AFTER delivery_times

There's s a typo here, should be additional_delivery_times instead

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 123 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

We need to use the long array syntax until 1.7.4, please change it everywhere 🙂

Change in this file


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 123 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

We need to use the long array syntax until 1.7.4, please change it everywhere 🙂

Change in this file


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

No need for onclick=""

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

No need for onclick=""

Done.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Wouldn't it be simpler to extract the <a> and the <i> from the translation? I don't see any need to keep it as part of the translation itself.

<a href="{{ getAdminLink("AdminPPreferences") }}" class="btn sensitive px-0"><i class="material-icons">open_in_new</i>
{{ "edit"|trans({}, 'Admin.Catalog.Help') }}</a>

Done.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Resources/views/Admin/Product/Include/form_shipping.html.twig, line 87 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Wouldn't it be simpler to extract the <a> and the <i> from the translation? I don't see any need to keep it as part of the translation itself.

<a href="{{ getAdminLink("AdminPPreferences") }}" class="btn sensitive px-0"><i class="material-icons">open_in_new</i>
{{ "edit"|trans({}, 'Admin.Catalog.Help') }}</a>

Done.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


classes/Product.php, line 262 at r2 (raw file):

Previously, kompilorb wrote…

Not at this moment

Isn't it possible to create class constants ?


classes/Product.php, line 271 at r3 (raw file):

     * Delivery in-stock information
     *
     * Long description for delivery in-stock product information.

Are we really talking about a description ? It looks more like a time range for delivery.


classes/Product.php, line 280 at r3 (raw file):

     * Delivery out-stock information
     *
     * Long description for delivery out-stock product information.

Are we really talking about a description ? It looks more like a time range for delivery.


src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

            'label' =>
               $this->translator->trans(

Should be on the same line


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


classes/Product.php, line 262 at r2 (raw file):

Previously, kompilorb wrote…

Not at this moment

Isn't it possible to create class constants ?


classes/Product.php, line 271 at r3 (raw file):

     * Delivery in-stock information
     *
     * Long description for delivery in-stock product information.

Are we really talking about a description ? It looks more like a time range for delivery.


classes/Product.php, line 280 at r3 (raw file):

     * Delivery out-stock information
     *
     * Long description for delivery out-stock product information.

Are we really talking about a description ? It looks more like a time range for delivery.


src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

            'label' =>
               $this->translator->trans(

Should be on the same line


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 17, 2017

Member

:lgtm:


Reviewed 2 of 6 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

Member

eternoendless commented Oct 17, 2017

:lgtm:


Reviewed 2 of 6 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 271 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Are we really talking about a description ? It looks more like a time range for delivery.

Yes it's a description, example is a time range yes, but customer can write any description about delivery information and not only a time range.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 271 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Are we really talking about a description ? It looks more like a time range for delivery.

Yes it's a description, example is a time range yes, but customer can write any description about delivery information and not only a time range.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 280 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Are we really talking about a description ? It looks more like a time range for delivery.

Yes it's a description, example is a time range yes, but customer can write any description about delivery information and not only a time range.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 280 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Are we really talking about a description ? It looks more like a time range for delivery.

Yes it's a description, example is a time range yes, but customer can write any description about delivery information and not only a time range.


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…
            'label' =>
               $this->translator->trans(

Should be on the same line

Why on the same line ?


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…
            'label' =>
               $this->translator->trans(

Should be on the same line

Why on the same line ?


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

Previously, kompilorb wrote…

Why on the same line ?

done


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

src/PrestaShopBundle/Form/Admin/Product/ProductShipping.php, line 137 at r3 (raw file):

Previously, kompilorb wrote…

Why on the same line ?

done


Comments from Reviewable

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 17, 2017

Contributor

classes/Product.php, line 262 at r2 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Isn't it possible to create class constants ?

I don't say that's not possible. I say only "not at this moment" this value must be use in legacy, smarty, symfony and twig. And i must ask for know if we do or not.


Comments from Reviewable

Contributor

kompilorb commented Oct 17, 2017

classes/Product.php, line 262 at r2 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Isn't it possible to create class constants ?

I don't say that's not possible. I say only "not at this moment" this value must be use in legacy, smarty, symfony and twig. And i must ask for know if we do or not.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Oct 24, 2017

Contributor

Hello @kompilorb
Some little things to change:
capture du 2017-10-24 16-41-26

The translation of these fields doesn't work:
capture du 2017-10-24 16-44-41

Contributor

marionf commented Oct 24, 2017

Hello @kompilorb
Some little things to change:
capture du 2017-10-24 16-41-26

The translation of these fields doesn't work:
capture du 2017-10-24 16-44-41

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 27, 2017

Contributor

tests are broken for now, this need an extra work to make this mergeable :)

Good job

Contributor

mickaelandrieu commented Oct 27, 2017

tests are broken for now, this need an extra work to make this mergeable :)

Good job

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 27, 2017

Contributor

@kompilorb ping me when tests are OK, I'll approve the pull request 👍

Contributor

mickaelandrieu commented Oct 27, 2017

@kompilorb ping me when tests are OK, I'll approve the pull request 👍

@kompilorb

This comment has been minimized.

Show comment
Hide comment
@kompilorb

kompilorb Oct 27, 2017

Contributor

@mickaelandrieu ping ! :)

Contributor

kompilorb commented Oct 27, 2017

@mickaelandrieu ping ! :)

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 30, 2017

Member

:lgtm:


Reviewed 9 of 9 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 30, 2017

:lgtm:


Reviewed 9 of 9 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Oct 30, 2017

Thank you @kompilorb

@eternoendless eternoendless merged commit 30f2d5a into PrestaShop:develop Oct 30, 2017

2 checks passed

code-review/reviewable 13 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kompilorb kompilorb deleted the kompilorb:1.7.3_productpage branch Nov 6, 2017

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