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

Migrate "Improve > Payment > Preferences" page #9225

Merged
merged 22 commits into from Jun 29, 2018

Conversation

@sarjon
Member

sarjon commented Jun 27, 2018

Questions Answers
Branch? develop
Description? This PR migrates Payment preferences page to symfony.
Type? improvement
Category? BO
BC breaks? yes (overrides)
Deprecations? no
Fixed ticket? n/a
How to test? You should be able to configure payment modules as you would in old page.

This change is Reviewable

@sarjon sarjon changed the title from [WIP] Migrate "Improve > Payment > Preferences" page to Migrate "Improve > Payment > Preferences" page Jun 27, 2018

/**
* Class PaymentModulePreferencesConfiguration is responsible for configuring payment module restrictions
*/
final class PaymentModulePreferencesConfiguration implements DataConfigurationInterface

This comment has been minimized.

@sarjon

sarjon Jun 27, 2018

Member

this class depends on interfaces from Core namespace, maybe it should be moved to Core namespace as well?
@mickaelandrieu what would be the correct location for it? Maybe Core/Configuration/Payment?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 29, 2018

Contributor

you're right I do the update

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Jun 27, 2018

@marionf marionf self-assigned this Jun 28, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 28, 2018

Hello @sarjon

When I have no payments modules installed, I have this issue:

capture du 2018-06-28 11-51-41

I should have the following message instead: "No payment module installed"

*/
private function sortPaymentModules(array $paymentModules)
{
foreach ($paymentModules as $key => $paymentModule) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 28, 2018

Contributor

undeclared variable $sortingBy

This comment has been minimized.

@sarjon

sarjon Jun 28, 2018

Member

🤦‍♂️ that fixes the issue.

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 28, 2018

@marionf it's fixed now, thanks for quick feedback!

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 28, 2018

Thank you @sarjon

Sorry, but I just found another little thing :
I have installed Paypal module and the radio buttons are not very pretty

capture du 2018-06-28 17-14-50

Do you think we can improve that ?

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 28, 2018

i know, it does not look good to me either. :/
Unfortunately, i cannot find any example of how radio should look like in PrestaKit. I've also checked Catalog page in Backoffice and it uses this ugly radios as well. :/

maybe @mickaelandrieu you know?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

@sarjon can you ensure we have the right markup here?

https://getbootstrap.com/docs/4.0/components/forms/#default-stacked

Afaik we don't have extra customization that comes from the UI Kit.

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 28, 2018

@sarjon can you ensure we have the right markup here?

not exactly the same markup, but as i can see in Boostrap 4 docs, they look the same.

Edit: the radio markup itself comes from RadioType, i didnt use any custom template here. :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

Ok, sorry @marionf this is not something we can improve here, but in the PrestaShop UI kit :/

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 28, 2018

All right, I will see with @TristanLDD :)

@NadiaCamard

This comment has been minimized.

NadiaCamard commented Jun 28, 2018

Hello, for the radio button, can you please use these material icons below?

<i class="material-icons"> radio_button_checked </i>

<i class="material-icons"> radio_button_unchecked </i>

More info: https://material.io/tools/icons/?style=baseline

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 28, 2018

@NadiaCamard sure, but this will be done in another contribution 👍

Note the changes you're asking will have an impact on every radio button of the Back Office: are you ok with that?

@NadiaCamard

This comment has been minimized.

NadiaCamard commented Jun 29, 2018

@mickaelandrieu thanks! that's more than ok if we harmonize the design of this component!

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 29, 2018

Hello @sarjon why is it a radio button and not a checkbox input here?

@sarjon

This comment has been minimized.

Member

sarjon commented Jun 29, 2018

why is it a radio button and not a checkbox input here?

it depends on currencies_mode which is attribute of PaymentModuleCore. It can have values checkbox or radio.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 29, 2018

Thanks @sarjon and @marionf for the review!

@mickaelandrieu mickaelandrieu merged commit 9a1e3cf into PrestaShop:develop Jun 29, 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 mickaelandrieu referenced this pull request Jun 29, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@matks matks added the migration label Sep 18, 2018

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