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

Update modern forms layout #9430

Merged
merged 15 commits into from Aug 21, 2018

Conversation

@sarjon
Member

sarjon commented Aug 10, 2018

Questions Answers
Branch? develop
Description? Update modern forms with new layout
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? See the list below with updated pages that form layout has been changed.

List of updated pages:

  • Shop Parameters > Maintenance
  • Shop Parameters > Customer Settings
  • Shop Parameters > Order Settings
  • Shop Parameters > Product Settings
  • Advanced Parameters > Performance
  • Advanced Parameters > Administration
  • Advanced Parameters > E-mail
  • International > Localization
  • International > Geolocation
  • Shipping > Preferences
  • Orders > Delivery Slips
  • Orders > Invoices

This change is Reviewable

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 10, 2018

Member

@eternoendless is this correct update? am i missing anything? if not, ill update other pages as well. :)

Member

sarjon commented Aug 10, 2018

@eternoendless is this correct update? am i missing anything? if not, ill update other pages as well. :)

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Aug 10, 2018

Member

Almost! Check out the changes in this diff: https://github.com/PrestaShop/PrestaShop/pull/9397/files?utf8=%E2%9C%93&diff=split&w=1#diff-bc4c4faa966c237b7346ad189fdda6de

The column containing the card must be col-xl-10, the card-block is missing row, and the whole block itself must be aligned to center.

Other than that, its 👍

Member

eternoendless commented Aug 10, 2018

Almost! Check out the changes in this diff: https://github.com/PrestaShop/PrestaShop/pull/9397/files?utf8=%E2%9C%93&diff=split&w=1#diff-bc4c4faa966c237b7346ad189fdda6de

The column containing the card must be col-xl-10, the card-block is missing row, and the whole block itself must be aligned to center.

Other than that, its 👍

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 11, 2018

Member

@eternoendless i've updated a few more pages, and here's what i noticed:

  1. When testing responsive i noticed that there's no more margin from left & right, is it intentional?
    responsive_no_margin

  2. Some labels a too long so help icon jumps below text.
    help_position

  3. In Performance page there's Caching block, it has very specific layout and thus does not look good after update. I could fix it it another PR if needed.

Other than that, all pages looks good with consistent layout on both desktop & mobile screens. 👍

Member

sarjon commented Aug 11, 2018

@eternoendless i've updated a few more pages, and here's what i noticed:

  1. When testing responsive i noticed that there's no more margin from left & right, is it intentional?
    responsive_no_margin

  2. Some labels a too long so help icon jumps below text.
    help_position

  3. In Performance page there's Caching block, it has very specific layout and thus does not look good after update. I could fix it it another PR if needed.

Other than that, all pages looks good with consistent layout on both desktop & mobile screens. 👍

<div class="card-text">
<div class="row">
<div class="col">
{{ ps.infotip('CCC allows you to reduce the loading time of your page. With these settings you will gain performance without even touching the code of your theme. Make sure, however, that your theme is compatible with PrestaShop 1.4+. Otherwise, CCC will cause problems.'|trans({}, 'Admin.Advparameters.Help')) }}

This comment has been minimized.

@sarjon

sarjon Aug 11, 2018

Member

is this part of message relevant in 1.7 anymore? I think we could delete it, right?

"Make sure, however, that your theme is compatible with PrestaShop 1.4+. Otherwise, CCC will cause problems."

@sarjon

sarjon Aug 11, 2018

Member

is this part of message relevant in 1.7 anymore? I think we could delete it, right?

"Make sure, however, that your theme is compatible with PrestaShop 1.4+. Otherwise, CCC will cause problems."

This comment has been minimized.

@eternoendless

eternoendless Aug 13, 2018

Member

I think you can delete it, but what do you think @colinegin ? It will need to be re-traduced if we change it.

@eternoendless

eternoendless Aug 13, 2018

Member

I think you can delete it, but what do you think @colinegin ? It will need to be re-traduced if we change it.

This comment has been minimized.

@colinegin

colinegin Aug 20, 2018

Ok to remove it !
@LouiseBonnard do you think we should add something else instead ?

@colinegin

colinegin Aug 20, 2018

Ok to remove it !
@LouiseBonnard do you think we should add something else instead ?

@mickaelandrieu mickaelandrieu changed the title from Update SF forms layout to Update modern forms layout Aug 13, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Aug 13, 2018

Member
  1. There are still margins when in responsive mode, but they are drastically reduced. This is per design.
  2. As per design too, tips should be placed below the field (check out the shop parameters for guidance). I think we should only use the pop over buttons when there isn't enough place for tips below fields (sorry I forgot about that).
  3. Ok but what's the issue exactly?
Member

eternoendless commented Aug 13, 2018

  1. There are still margins when in responsive mode, but they are drastically reduced. This is per design.
  2. As per design too, tips should be placed below the field (check out the shop parameters for guidance). I think we should only use the pop over buttons when there isn't enough place for tips below fields (sorry I forgot about that).
  3. Ok but what's the issue exactly?
@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 13, 2018

Member

Ok but what's the issue exactly?

this is how it looks https://prnt.sc/ki5chd

Member

sarjon commented Aug 13, 2018

Ok but what's the issue exactly?

this is how it looks https://prnt.sc/ki5chd

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Aug 14, 2018

Member

Ok then, we can fix that in a separate PR. I think we'll need some cues from design for that... cc @TristanLDD

Member

eternoendless commented Aug 14, 2018

Ok then, we can fix that in a separate PR. I think we'll need some cues from design for that... cc @TristanLDD

<button class="btn btn-primary">{{ 'Save'|trans({}, 'Admin.Actions') }}</button>
</div>
<div class="form-group row">
{{ ps.label_with_help(('Display remaining quantities when the quantity is lower than'|trans), ('Set to "0" to disable this feature.'|trans({}, 'Admin.Shopparameters.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Both strings exist (Set to "0" to disable this feature. and Set to 0 to disable this feature.), perhaps we should just keep one instead of two with the same meaning? I prefer to keep it without quotation marks.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Both strings exist (Set to "0" to disable this feature. and Set to 0 to disable this feature.), perhaps we should just keep one instead of two with the same meaning? I prefer to keep it without quotation marks.

<div class="card-block row">
<div class="card-text">
<div class="form-group row">
{{ ps.label_with_help(('Allow ordering of out-of-stock products'|trans), ('By default, the Add to Cart button is hidden when a product is unavailable. You can choose to have it displayed in all cases.'|trans({}, 'Admin.Shopparameters.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

It is written "add to cart" button above (cf. Display the "add to cart" button when a product has attributes), it would be better to harmonize the wording and stay consistent. Let's keep the first option?

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

It is written "add to cart" button above (cf. Display the "add to cart" button when a product has attributes), it would be better to harmonize the wording and stay consistent. Let's keep the first option?

</div>
</div>
<div class="form-group row">
{{ ps.label_with_help(('Maintenance IP'|trans), ('IP addresses allowed to access the front office even if the shop is disabled. Please use a comma to separate them (e.g. 42.24.4.2,127.0.0.1,99.98.97.96)'|trans({}, 'Admin.Shopparameters.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Same here, we have eg: written above as well as e.g. in this PR, it should be the second option.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Same here, we have eg: written above as well as e.g. in this PR, it should be the second option.

<div class="card-text">
<div class="form-group">
<div class="form-group row">
{{ ps.label_with_help(('Language identifier'|trans), ('The ISO 639-1 identifier for the language of the country where your web server is located (en, fr, sp, ru, pl, nl, etc.).'|trans({}, 'Admin.International.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Actually, there is like a very silly mistake in this string... in the enumeration part, I guess sp stands for the Spanish language, but this 639-1 code does not exist, it should be es.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Actually, there is like a very silly mistake in this string... in the enumeration part, I guess sp stands for the Spanish language, but this 639-1 code does not exist, it should be es.

</div>
<div class="form-group">
<div class="form-group row">
{{ ps.label_with_help(('Country identifier'|trans), ('The ISO 3166-1 alpha-2 identifier for the country/region where your web server is located, in lowercase (us, gb, fr, sp, ru, pl, nl, etc.).'|trans({}, 'Admin.International.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Are we sure that we need the country code to be written in lowercase?

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Are we sure that we need the country code to be written in lowercase?

</div>
<div class="form-group row">
{{ ps.label_with_help(('Delivery Number'|trans), ('The next delivery slip will begin with this number and then increase with each additional slip.'|trans({}, 'Admin.Orderscustomers.Help'))) }}

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Delivery number instead, in lowercase. ;-)

@LouiseBonnard

LouiseBonnard Aug 14, 2018

Contributor

Delivery number instead, in lowercase. ;-)

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 17, 2018

Contributor

Ping @sarjon this one also needs your attention :)

Contributor

mickaelandrieu commented Aug 17, 2018

Ping @sarjon this one also needs your attention :)

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 17, 2018

Member

@mickaelandrieu in this PR translations were not changed, it only updated styling. Should i change translations as well? It seems as it could be better done in another PR (which could also remove any translations with messages domain).

Member

sarjon commented Aug 17, 2018

@mickaelandrieu in this PR translations were not changed, it only updated styling. Should i change translations as well? It seems as it could be better done in another PR (which could also remove any translations with messages domain).

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 20, 2018

Contributor

@sarjon ok for me.

@LouiseBonnard update of migration keys that are currently in messages domain will be adressed in another pull request 👍 thanks for the review

Contributor

mickaelandrieu commented Aug 20, 2018

@sarjon ok for me.

@LouiseBonnard update of migration keys that are currently in messages domain will be adressed in another pull request 👍 thanks for the review

@marionf marionf assigned marionf and unassigned marionf Aug 20, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 21, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Aug 21, 2018

@mickaelandrieu mickaelandrieu merged commit b6e2b54 into PrestaShop:develop Aug 21, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

Thanks everyone!

Contributor

mickaelandrieu commented Aug 21, 2018

Thanks everyone!

@matks matks added the migration label Sep 19, 2018

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