ECO-2309 FE module integration into suite #24

sergeirybalko · 2018-12-18T13:50:13Z

TICKET: https://spryker.atlassian.net/browse/ECO-2309

tolerants · 2018-12-20T12:41:16Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+    protected payConfig: IAmazonConfig;
+
+    protected readyCallback(): void {
+        this.loginScopeOptions = <string>this.loginScopeOptionsAttribute;


Suggest to rename method loginScopeOptionsAttribute into loginScopeOptions and remove this property.

tolerants · 2018-12-20T12:41:22Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+
+    protected readyCallback(): void {
+        this.loginScopeOptions = <string>this.loginScopeOptionsAttribute;
+        this.loginPopupOptions = <string>this.loginPopupOptionsAttribute;


The same here.

tolerants · 2018-12-20T13:48:46Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+    }
+
+    protected amazonLoginReady(): void {
+        (<any>window).onAmazonLoginReady = () => {


Suggest to add window declaration to avoid specifying of any type each time.
declare var window: any;

tolerants · 2018-12-20T15:59:39Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+    }
+
+    protected amazonPaymentsReady(loginScopeOptions: string, loginPopupOptions: string, redirectUrl: string): void {
+        let addressConsentToken;


Why the name is addressConsentToken? How it is related to address? Maybe accessToken will be better?

tolerants · 2018-12-20T16:03:43Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+        this.loginScopeOptions = <string>this.loginScopeOptionsAttribute;
+        this.loginPopupOptions = <string>this.loginPopupOptionsAttribute;
+        this.payConfig = <IAmazonConfig> {
+            clientId: this.clientIdAttribute,


Suggest to remove Attribute. Please do it everywhere.

tolerants · 2018-12-20T16:07:01Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.ts

+                        (<any>window).errorHandler(new Error('referenceId missing'));
+                        return;
+                    }
+                    (<any>window).location = redirectUrl + '?'


I think template sting will be useful here.

tolerants · 2018-12-20T16:10:06Z

...prykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.twig

+    clientId: required,
+    sellerId: required,
+    logout: required,
+    redirectUrl: '',


Are you sure it's not required? I cann't see any validation in TS implementation for it.

tolerants · 2018-12-20T18:50:47Z

...Yves/AmazonPay/Theme/default/components/atoms/get-shipment-methods/get-shipment-methods.twig

@@ -0,0 +1,36 @@
+{% extends model('component') %}


I think this is molecule, not atom.

tolerants · 2018-12-20T18:56:02Z

...prykerEco/Yves/AmazonPay/Theme/default/components/atoms/cart-pay-button/cart-pay-button.twig

+} %}
+
+{% block body %}
+    <div id="{{ config.jsName }}__item" align="center"></div>


Please use class for alignment.

tolerants · 2018-12-21T08:01:41Z

...Yves/AmazonPay/Theme/default/components/atoms/get-shipment-methods/get-shipment-methods.twig

+                           class="radio"
+                           name="amazonpayShipmentMethod"
+                           value="{{ shipmentMethod.idShipmentMethod }}"
+                        {% if data.selectedShipmentMethodId == shipmentMethod.idShipmentMethod %} checked="checked"{% endif %}


checked attribute doesn't need a value.

tolerants · 2018-12-21T08:01:54Z

...Yves/AmazonPay/Theme/default/components/atoms/get-shipment-methods/get-shipment-methods.twig

+                           name="amazonpayShipmentMethod"
+                           value="{{ shipmentMethod.idShipmentMethod }}"
+                        {% if data.selectedShipmentMethodId == shipmentMethod.idShipmentMethod %} checked="checked"{% endif %}
+                        {% if data.isAmazonPaymentInvalid %} disabled="disabled"{% endif %}


The same for disabled.

tolerants · 2018-12-21T08:35:15Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/molecules/payment-step/payment-step.ts

+        this.orderReferenceAjaxProvider = <AjaxProvider>this.querySelector(`.${this.jsName}__order-reference-ajax-provider`);
+        this.shipmentUpdateAjaxProvider = <AjaxProvider>this.querySelector(`.${this.jsName}__shipment-update-ajax-provider`);
+        this.payConfig = <IAmazonConfig> {
+            sellerId: this.sellerIdAttribute,


Please remove Attribute word for all getters.

I removed all Attribute world for all getters and from cart-pay-button.ts also

tolerants · 2018-12-23T11:53:20Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/molecules/payment-step/payment-step.ts

+                    const referenceId = orderReference.getAmazonOrderReferenceId();
+                    const formData = new FormData();
+                    formData.append('reference_id', referenceId);
+                    _this.orderReference(formData);


Is undefined value handles correctly?

I do not know, because I did not watch specification Amazon Pay. I think this moment need conversation with @profuel

tolerants · 2018-12-23T15:24:26Z

...erEco/Yves/AmazonPay/Theme/default/components/molecules/shipment-method/shipment-method.twig

+                            {% endif %}
+                        )
+                    </strong>
+                    <span class="float-right">


Suggest to move span inside if condition.

tolerants · 2018-12-23T15:24:59Z

...erEco/Yves/AmazonPay/Theme/default/components/molecules/shipment-method/shipment-method.twig

+            {% if data.quoteTransfer.shipment and data.quoteTransfer.shipment.method %}
+                <li>
+                    {{ 'checkout.step.summary.shipping' | trans }}
+                    <strong>


Suggest to move strong inside if condition.

tolerants · 2018-12-23T15:29:56Z

src/SprykerEco/Yves/AmazonPay/Theme/default/views/checkout/checkout.twig

+                    </ul>
+                {% endif %}
+
+                {% if data.cartItems is defined %}


It's always defined, change condition to is not empty.

tolerants · 2018-12-27T08:21:20Z

src/SprykerEco/Yves/AmazonPay/Theme/default/components/molecules/payment-step/payment-step.ts

-            locale: this.localeAttribute,
-            orderReferenceId: (this.orderReferenceIdAttribute === "") ? undefined : this.orderReferenceIdAttribute,
-            displayMode: (this.displayModeAttribute === "") ? undefined : this.displayModeAttribute
+             sellerId: this.sellerId,


It looks like you have 5 spaces indention here.

…co-2290/eco-2309-fe-module-integration

sergeirybalko added 5 commits December 13, 2018 14:18

ECO-2390: Commit before pull

48bcac1

ECO-2390: Commit before pull

18ca715

ECO-2309: commit before stash

8af2de1

ECO-2309: Partial job complete

54897d3

ECO-2309: AmazonPay complte and ready for CR

7e1db57

tolerants requested changes Dec 20, 2018

View reviewed changes

ECO-2285: Change code after code review

3714dd4

tolerants requested changes Dec 23, 2018

View reviewed changes

ECO-2309: Change code after code review

7aa2935

tolerants approved these changes Dec 27, 2018

View reviewed changes

Merge branch 'feature/eco-2286-amazon-pay-integration' into feature/e…

2dbc299

…co-2290/eco-2309-fe-module-integration

profuel approved these changes Jan 10, 2019

View reviewed changes

profuel merged commit 5fd342a into feature/eco-2286-amazon-pay-integration Jan 10, 2019

profuel deleted the feature/eco-2290/eco-2309-fe-module-integration branch January 10, 2019 08:30

profuel restored the feature/eco-2290/eco-2309-fe-module-integration branch January 10, 2019 09:07

profuel deleted the feature/eco-2290/eco-2309-fe-module-integration branch January 10, 2019 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECO-2309 FE module integration into suite #24

ECO-2309 FE module integration into suite #24

sergeirybalko commented Dec 18, 2018 •

edited

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 20, 2018

sergeirybalko Dec 21, 2018

tolerants Dec 21, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 21, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 21, 2018

sergeirybalko Dec 24, 2018 •

edited

tolerants Dec 23, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 23, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 23, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 23, 2018

sergeirybalko Dec 24, 2018

tolerants Dec 27, 2018

ECO-2309 FE module integration into suite #24

ECO-2309 FE module integration into suite #24

Conversation

sergeirybalko commented Dec 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeirybalko Dec 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeirybalko commented Dec 18, 2018 •

edited

sergeirybalko Dec 24, 2018 •

edited