Add Shipping settings to Cart block #1561

senadir · 2020-01-14T16:54:05Z

I've added two toggle options, show calculator and hide costs.
I've also moved getTotalRowsConfig inside FullCart since it depends on an attribute passed from the above block, we can keep it out and just add an extra param to getTotalRowsConfig.
Hide the options on Empty cart mode.
Render those changes to the frontend

Screenshots

senadir · 2020-01-14T16:56:36Z

One note that I didn’t check yet, if we’re hiding the shipping costs, should we do something about removing it from the API response? or should we leave mismatching values?

Example of mismatched values

Aljullu · 2020-01-15T09:46:28Z

@senadir is this ready for review? I see it's in the Review/QA pipeline and it has the Needs review tag but it's a draft PR.

senadir · 2020-01-15T09:47:59Z

@Aljullu yes and no, while it doesn’t link the option source to wcSettings yet, I would love if you can review the settings boilerplate in general

see

Fixes #1488 I'm not yet fully sure how we should wrap or present those settings or how to handle global default values, so until we finish the discussion in #1488 I'm keeping this a draft.

Aljullu

Fixes #1488 I'm not yet fully sure how we should wrap or present those settings or how to handle global default values, so until we finish the discussion in #1488 I'm keeping this a draft.

Sorry, I missed that comment, initially. 😅

I think we can work on that in a follow-up so this PR is not blocked by that. That will make the block to invalidate if we change the attributes, but as long as we fix #1488 before release we shouldn't bother about it.

The PR is looking great, just some comments:

I wonder if we should take the chance to make both settings positive, so instead of Hide shipping costs it's Shipping costs:
Looking at the old Cart, the string (change address) seems to be part of the Shipping Calculator, so it should be visible/hidden depending on the Shipping calculator setting instead of the Hide shipping costs. I'm ok fixing it in a follow-up, though. Since that will cause conflicts with Add ShippingCalculator component #1559 anyway.

assets/js/blocks/cart-checkout/cart/edit.js

haszari

Added some code comments which may or may not be helpful. I think the best way to unblock this PR is to focus on:

Persisting the settings in block attributes.
Applying the settings in the front end render. I tested the front end and the settings aren't applying, though I'm guessing that's as intended (draft PR).

The next step would be correctly defaulting these attributes based on the woo core settings, which could be part of this PR or a follow up.

Might be worth focusing this PR on the happy case – site has shipping settings/zones all set up. I see in the design there's guidance for what to do when there are no shipping methods/zones, which could potentially be a follow up PR.

I think it's worth revisiting the wording of the options themselves (cc @garymurray @nerrad @pmcpinto), though possibly that can happen in a follow up PR. I thought I understood these options when initially reviewing the cart design, but looking at them now I'm confused :)

haszari · 2020-01-16T03:09:56Z

assets/js/blocks/cart-checkout/cart/full-cart/index.js

-const Cart = () => {
-	const currency = getCurrencyFromPriceResponse( cartTotals );
-	const totalRowsConfig = getTotalRowsConfig( cartTotals );
+const Cart = ( { isShippingCalculatorEnabled, isShippingCostHidden } ) => {


Is there some whitespace issue that you're fixing here too? This diff is really big. (I reviewed with Hide whitespace changes for now.)

It’s mostly moving the getTotalRowsConfig function to inside the Cart component since it depends on attributes based to Cart, mainly isShippingCostHidden

Ah that makes sense :)

I have a slight preference for adding the param to getTotalRowsConfig, so its dependencies are explicit, and less impact on the code. Not a blocker, happy for this to merge as is :)

(There's some fun merging coming with this and #1741 both close to merge!)

assets/js/blocks/cart-checkout/cart/edit.js

haszari · 2020-01-16T03:15:57Z

assets/js/blocks/cart-checkout/cart/full-cart/index.js

 // @todo this are placeholders
 const onActivateCoupon = ( couponCode ) => {
 	// eslint-disable-next-line no-console
 	console.log( 'coupon activated: ' + couponCode );
 };
-const cartTotals = {
+const fetchedCartTotals = {


Is this inline test data needed? We have previews/cart-items.js which should contain a valid API response. If you need different data for testing, consider editing the preview data.

this was in the file when I started working on it so I didn’t feel like changing things, but I think you’re right, however the previews/cart-items had only items values, not total.

Gotcha. Could potentially add this test data to a new previews/cart to match the store/cart endpoint. But yep I see that I'm commenting on stuff that came in a previous PR, apologies :)

FYI @senadir I've shifted the preview cart totals data as part of hooking up cart to API in PR #1741 (in progress), see this commit. (Apologies, this will cause conflicts!)

97cda2b

I can help you rebase if this was merged before your, or I can rebase mine, no problem

haszari · 2020-01-16T03:16:58Z

assets/js/blocks/cart-checkout/cart/full-cart/index.js

+		}
+		const totalDiscount = parseInt( cartTotals.total_discount, 10 );
+		if ( totalDiscount > 0 ) {
+			const totalDiscountTax = parseInt(


Should this be in this PR? Looks unrelated to shipping options to me :)

This was here when I started editing, it only shows up as edited since I moved the function from outside the component to inside it, the changes I made in assets/js/blocks/cart-checkout/cart/full-cart/index.js are adding two props, and adding a condition to certain items according to those props.

garymurray · 2020-01-16T09:39:54Z

To clarify what each setting does:

Shipping calculator

This one removes the shipping calculator from the cart completely, which makes the setting below it not needed, so if you disabled this, the setting below (Shipping costs) should be removed.

Shipping costs

If this setting is enabled, then the shipping costs are not shown automatically - the customer has to first enter and address to get the shipping costs to display.

senadir · 2020-01-16T11:19:23Z

I wonder if we should take the chance to make both settings positive, so instead of Hide shipping costs it's Shipping costs:

Definitely! I remember I brought this up once, double negation seems hard to comprehend cc @garymurray

senadir · 2020-01-16T11:43:33Z

I addressed some comments here, and replied to other, this PR still needs some work, like showing shipping costs once an addressed is entred, once Albert merges his shipping calculator PR, we can try something like this.
btw, the logic is really getting complicated once we start to juggle so many booleans

const [ showShippingCosts, setShowShippingCosts ] = useState( ! isShippingCostHidden );

useEffect( () => {
	if ( isShippingCalculatorEnabled ) {
		if ( isShippingCostHidden ) {
			if ( shippingAddress ) {
				return setShowShippingCosts( true );
			}
		} else {
			return setShowShippingCosts( true );
		}
	}
	return setShowShippingCosts( false );
}, [ isShippingCalculatorEnabled, isShippingCostHidden, shippingAddress ] );

garymurray · 2020-01-16T11:48:16Z

@senadir @Aljullu

I wonder if we should take the chance to make both settings positive, so instead of Hide shipping costs it's Shipping costs:

Definitely! I remember I brought this up once, double negation seems hard to comprehend cc @garymurray

Does this align with what you saying:

So by default they both toggle on, if you toggle of the first one, the second one is removed automatically as it no longer applies, but you can optionally leave the first one enabled and disable the second one, which would require the customer to enter their address before seeing the shipping cost.

nerrad · 2020-01-16T12:32:02Z

So by default they both toggle on, if you toggle of the first one, the second one is removed automatically as it no longer applies,

If we want parity with the existing settings, this is not consistent with them. I found the best way to understand these options is to try the existing ones in the WooCommerce settings with the existing (not new) cart. I tested with a flat shipping cost (vs a geographical zone), so there may be some variations of behaviour depending on those.

garymurray · 2020-01-16T13:42:38Z

If we want parity with the existing settings, this is not consistent with them.

@nerrad yes, the way they are shown in the screenshot above is now counter to what is in Core currently:

But the result is the same - the idea shared here was to see if it solved the double negation issue.

The original design was as per the core settings - but in block format.

senadir · 2020-01-21T12:27:53Z

The new commits hooks up the settings to Woo Core and render them in the frontend, it also rebases to the latest changes.

We have a problem in which withFeedback would jump above the settings after the first change to settings, is this something you can look into @Aljullu

Aljullu

Thanks for updating this PR @senadir and hooking up the attributes to the store settings!

Some comments/questions:

It's not clear to me if we agreed to make both settings "positive", Gary's designs look like so but that's not implemented yet, right?
We also have the issue of the (change address) string. It belongs to the Shipping Calculator but currently it's hidden with the Shipping Costs toggle. As I said in my previous review, I think it's ok dealing with it in a follow-up, but we should either create a follow-up issue for it or not mark this PR as fixing Cart Block: Block settings for shipping options. #1488. Otherwise we will probably forget about it. 😅
I left a couple of small comments below in the code. 👇

We have a problem in which withFeedback would jump above the settings after the first change to settings, is this something you can look into @Aljullu

I'm unable to reproduce this. When does it happen?

Aljullu · 2020-01-21T14:20:23Z

assets/js/settings/blocks/constants.js

+	'isShippingCalculatorEnabled',
+	true
+);
+export const HIDE_SHIPPING_COST = getSetting( 'isShippingCostHidden', false );


I think we should name these constants IS_SHIPPING_CALCULATOR_ENABLED and IS_SHIPPING_COST_HIDDEN, that would keep them consistent with the settings and make it clear they are booleans.

I agree, let's use the same convention for naming these, so it's clear that they are a value (not an action) and boolean type.

Aljullu · 2020-01-21T14:24:12Z

assets/js/blocks/cart-checkout/cart/edit.js

@@ -37,14 +90,20 @@ const CartEditor = ( { className } ) => {
 				] }
 				defaultView={ 'full' }
 				render={ ( currentView ) => (
-					<Fragment>
+					<>


I don't have any preference, but we are always explicitly writing Fragment in tags. Should we keep it consistent? We could even add an eslint rule for it.

I have a preference for fewer keystrokes and fewer imports, but If we're always using Fragment then I can switch back to it.

Yeah, makes sense... We could open an issue and discuss it there, I just feel that keeping it consistent makes code easier to scan, whichever way we go.

I'd vote for the more explicit <Fragment> here, but this doesn't need to block the PR.

I know I'm contradicting myself 🙂 but in another PR we were discussing if the short syntax (<>) would be better.

I created an issue to discuss/switch: #1761, this way we don't block current open PRs because of that.

senadir · 2020-01-21T14:55:57Z

It's not clear to me if we agreed to make both settings "positive", Gary's designs look like so but that's not implemented yet, right?

As per Darren and Gary, I feel like parity with Woo Core is more important here, removing double negation could start from Woo Core and get bubbled to us later.

We also have the issue of the (change address) string. It belongs to the Shipping Calculator but currently it's hidden with the Shipping Costs toggle. As I said in my previous review, I think it's ok dealing with it in a follow-up, but we should either create a follow-up issue for it or not mark this PR as fixing #1488. Otherwise we will probably forget about it.

I don’t think I'm following, the current behavior will hide Shipping to location (change address) when shipping calculator gets disabled, shipping costs do not affect that one

senadir · 2020-01-21T15:02:27Z

We have a problem in which withFeedback would jump above the settings after the first change to settings, is this something you can look into

I'm unable to reproduce this. When does it happen?

try toggling off or on a settings

Aljullu · 2020-01-21T18:20:05Z

I think I didn't pull before re-testing it, sorry for the confusion @senadir! 🤦‍♂️

try toggling off or on a settings

Yes, I could reproduce it now. It looks like the order inside InspectorControls changes with the re-render? I don't have a big slot-fill knowledge so I'm not sure if there is an easy solution for it. One approach would be to avoid using the HOC and directly insert the <FeedbackPrompt> component in BlockSettings, but it might be worth exploring if there are other approaches. In any case, we should open an issue to track that. I can do it if you want.

I don’t think I'm following, the current behavior will hide Shipping to location (change address) when shipping calculator gets disabled, shipping costs do not affect that one

Oh ok, nvm. I was confused about what was the calculator and what were the costs.

One thing that still confuses me: I assumed the Shipping: $1.23 (first line in the screenshot) was not part of the calculator but part of the shipping costs:

senadir · 2020-01-21T18:41:19Z

One thing that still confuses me: I assumed the Shipping: $1.23 (first line in the screenshot) was not part of the calculator but part of the shipping costs.

Yeah, that’s part of the testing data, my main point of confusion is, should the API do this, or should the setting code be responsible for the total

mikejolley · 2020-02-14T11:41:51Z

@senadir Is this awaiting further review, or revisions? There are some conflicts also.

haszari

The recent fixes look great, like the use of conventional filter_var for the bool option, and decreased nesting with ShippingCalculatorOptions factored out.

Side note: I see that getTotalRowsConfig is inline in master now, using cartTotals from closure (no param) but still passing param in call (const totalRowsConfig = getTotalRowsConfig( cartTotals );). So that shouldn't block this PR, but we might want to tidy up cc @Aljullu :)

I'm still seeing the validation error issue, and a new issue on the front end.

Validation error

To reproduce:

Add cart block to a page, save.
Visit WooCommerce > Settings > Shipping settings page (/wp-admin/admin.php?page=wc-settings&tab=shipping&section=options).
Change shipping options defaults e.g. Enable the shipping calculator on the cart page, click Save Changes.
Reopen editor for page with cart block - see validation error.

Front end error

The shipping options aren't working at all for me, I think there's an issue getting the props out of the data attributes:

To reproduce:

Ensure you have some shipping options set up.
Add cart block to a page.
Enable Shipping calculator option (and disable Hide shipping costs). Should see shipping options in cart sidebar card.
Publish/update the page.
View page on front end – shipping options aren't shown.

senadir · 2020-02-19T12:55:57Z

Okay, so the latest round of commits includes:

fixing the issue of the attributes for good, the problem is that I wasn’t passing the attributes back in the frontend. e8fb026
Showing the shipping costs once an address is submitted (it only checks that you selected a country). 212a304 and 409ec62
Disables all shipping related things if shipping is not enabled c5e9419
fixes a side issue in coupons not being enabled by default 4bb114e

mikejolley

The settings code looks good bar one point (the display of the show shipping setting should not be tied to the calculator), but pre-approving.

We should log a follow up issue for handling these new options on the frontend views properly once we understand what they should do :)

mikejolley · 2020-02-19T16:14:40Z

assets/js/blocks/cart-checkout/cart/full-cart/index.js

+			);
+			totalRowsConfig.push( {
+				label: __( 'Shipping:', 'woo-gutenberg-products-block' ),
+				value: DISPLAY_PRICES_INCLUDING_TAXES


This value should be hidden if totals are hidden and there is no address yet, but can be a follow up if you want to log it.

mikejolley · 2020-02-19T16:16:02Z

assets/js/blocks/cart-checkout/cart/edit.js

+						} )
+					}
+				/>
+				{ isShippingCalculatorEnabled && (


This option should be independent of the calculator option, so you can remove the above check.

fixed and re-reviewed by Mike

senadir added status: needs review skip-changelog PRs that you don't want to appear in the changelog. labels Jan 14, 2020

senadir self-assigned this Jan 14, 2020

senadir requested review from Aljullu and nerrad January 14, 2020 16:54

Aljullu reviewed Jan 15, 2020

View reviewed changes

assets/js/blocks/cart-checkout/cart/edit.js Outdated Show resolved Hide resolved

haszari reviewed Jan 16, 2020

View reviewed changes

senadir marked this pull request as ready for review January 21, 2020 12:16

senadir requested a review from a team January 21, 2020 12:16

senadir force-pushed the add/settings-to-cart branch 2 times, most recently from d2f3c61 to f43c8e2 Compare January 21, 2020 12:37

Aljullu reviewed Jan 21, 2020

View reviewed changes

nerrad added this to the Future Release milestone Jan 27, 2020

This was referenced Feb 17, 2020

hook up cart items to API data in front end #1741

Merged

Cart Block (frontend context): Wire up cart block to use new endpoints #1489

Closed

haszari previously requested changes Feb 18, 2020

View reviewed changes

senadir force-pushed the add/settings-to-cart branch 2 times, most recently from 04c65ff to 4bb114e Compare February 19, 2020 12:49

senadir added 14 commits February 19, 2020 16:36

add cart settings

978a816

address typos

bcbc8c0

update how settings should work

cd86ec8

get settings from woo and pass them to frontend

77de0ba

reapply condition

08f55cb

fix issue with attributes not presisting

2c3092f

abstract shipping control

b49425e

rename constants

1436b58

update props in frontend

3dfd6db

fix bug with attributes not presisting

bb048cd

conditionally display prices

54009a5

use country to decide to show prices

609f10e

disable shipping if shipping is not enabled

f6db165

enable coupons by default

236538f

senadir force-pushed the add/settings-to-cart branch from 4bb114e to 236538f Compare February 19, 2020 15:36

mikejolley approved these changes Feb 19, 2020

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Feb 19, 2020

senadir merged commit 3251935 into master Feb 19, 2020

senadir deleted the add/settings-to-cart branch February 19, 2020 16:58

haszari mentioned this pull request Feb 24, 2020

Create typedefs and add basic typescript configuration #1796

Merged

nerrad modified the milestones: Future Release, 2.6.0 Apr 17, 2020

nerrad mentioned this pull request Apr 29, 2020

Cart - "Hide shipping costs" option wording mentions checkout #2343

Closed

Add Shipping settings to Cart block #1561

Add Shipping settings to Cart block #1561

Conversation

senadir commented Jan 14, 2020 • edited

Screenshots

senadir commented Jan 14, 2020

Aljullu commented Jan 15, 2020

senadir commented Jan 15, 2020

Aljullu left a comment

Choose a reason for hiding this comment

haszari left a 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

haszari Feb 17, 2020 • 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

garymurray commented Jan 16, 2020 • edited

senadir commented Jan 16, 2020

senadir commented Jan 16, 2020 • edited

garymurray commented Jan 16, 2020

nerrad commented Jan 16, 2020

garymurray commented Jan 16, 2020 • edited

senadir commented Jan 21, 2020

Aljullu left a 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

senadir commented Jan 21, 2020

senadir commented Jan 21, 2020

Aljullu commented Jan 21, 2020

senadir commented Jan 21, 2020

mikejolley commented Feb 14, 2020

haszari left a comment

Choose a reason for hiding this comment

Validation error

Front end error

senadir commented Feb 19, 2020

mikejolley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senadir commented Jan 14, 2020 •

edited

haszari Feb 17, 2020 •

edited

garymurray commented Jan 16, 2020 •

edited

senadir commented Jan 16, 2020 •

edited

garymurray commented Jan 16, 2020 •

edited