[MVP QA #1] Visual: spacings #38325

mdperez86 · 2023-05-16T20:23:07Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #38202

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Got to /wp-admin/tools.php?page=woocommerce-admin-test-helper
Under Features tab make sure to enable product-block-editor
Then visit /wp-admin/admin.php?page=wc-admin&path=/add-product
The product form should not have the problem described in [MVP QA #1] Visual: spacings #38202

github-actions · 2023-05-16T20:23:29Z

Hi @joshuatf,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2023-05-16T20:54:49Z

Test Results Summary

Commit SHA: ab0329f

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 50s
E2E Tests	194	0	0	10	0	204	21m 43s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

joshuatf

Looking great on my end! Thanks for the additional clean-up with notices in this PR.

Just left one question we discussed before. Let's see what @jarekmorawski says and we can update our naming accordingly.

joshuatf · 2023-05-26T02:00:50Z

packages/js/product-editor/src/blocks/section/editor.scss

+	}
+
+	&__content {
+		&--block-gap-lg > * + * {


joshuatf · 2023-05-26T02:02:49Z

packages/js/product-editor/src/blocks/section/block.json

+		},
+		"blockGap": {
+			"type": "string",
+			"enum": [ "lg", "2xlg" ],


We discussed this in our grooming call, but this is the only area the feels a little weird with the naming.

It would be great if we could match the gaps to our existing variable gap sizes (24px, 36px, 40px) and then match the naming there as well. @jarekmorawski is that feasible or do these gaps need to be different than our current gaps?

Meaning that we'd change all the gaps between fields and sections to those you listed? I'd prefer to avoid that. The ones in Figma are consistent with WordPress (more or less) and balance white space and density well. For example, increasing the gap between sections from 32 px to 36 px or 40 px will impact how much content is visible before scrolling.

By the way, where do our current gap sizes come from and can we adjust them to the new ones?

Those gaps came from the previous designers, but I'm unsure on the specifics of how they were chosen.

It would be really great to adjust them, though that would require changes in many places (a quick scan shows that we're using some form of the $gap variable in 762 files).

But if this is something we plan on doing one day, we could at least update the naming in this PR to match those. @jarekmorawski do you have ideal sizes for gaps/spacing for smallest, smaller, small, default, large, larger, and largest?

If we update the gaps, it'll affect all of wc-admin, correct? We'd have to do an audit to make sure we wouldn't break the visual rhythm outside the form, so I can't specify them yet. For the form, let's use the sizes below.

I'll leave it to your judgment and BEM best practices regarding naming.

XXS: 8px

XS: 16px

S: 24px

M: 32px

L: 40px

XL: 48px

XXL: 56px

We'll unlikely ever use gaps larger than 56 px for form elements or cards like in the Home screen. The only things I can think of are margins and paddings, but I'm not sure we should include them in the system.

If we update the gaps, it'll affect all of wc-admin, correct?

Correct, but we're not looking at tackling that in this PR. Mostly just curious if it's something we'll eventually make sizes consistent between these and the existing gaps. If so, we can match the naming and eventually replace them.

On the other hand, providing new sizes (xxs, xs, s, etc) provides us an easier way to transition over. I'm indifferent between those sizes and using the previous semantics (smallest, smaller, small, etc). @mdperez86 do you have an opinion on this?

@joshuatf in popular UI frameworks out there using (xs, sm, md, lg, xl) have become in a standard not only referring to a screen size but also to text-size and sometimes to spacing. But the problem this convention has is that the values are always fixed and we'll face the same problem in future if a new value appeared like now. Because of that some frameworks adopted a more scalable solution like using gap-1, gap-2, gap-3 -> 0.25rem /* 4px */, 0.5rem /* 8px */, 0.75rem /* 12px */ which give us infinite flexibility if a new value is required like 0.375rem /* 6px */ that can be represented as gap-1.5. Of course the downside of this approach is that is more difficult to recognize than a more descriptive names like xs or lg.
So I'm ok using the convention suggested by @jarekmorawski but just consider we could face the same problem we have now in the future.

@mdperez86 I like that style better as well and I think WooCommerce is the only one tied to a system using our current style of naming. I think that bodes well for us switching to a new relative gap system.

Looks like WordPress has adopted a similar approach.. And luckily we already have access to those variables here:

https://github.com/WordPress/gutenberg/blob/b2c16f3c684098a8dabff3911c610caa5c053701/packages/base-styles/_variables.scss#L31-L41

Let's follow that convention and we can slowly deprecate our previous gap system in time.

done -> ab0329f

…x and remove the line separating the header from the contents

…vertically misaligned. Left margin of 4 px, no vertical adjustment; somehow it has a -2px set for the top margin)

…d say Add, not Save. It should change into Update once it's published on the store

…onal

see: https://github.com/WordPress/gutenberg/blob/b2c16f3c684098a8dabff3911c610caa5c053701/packages/base-styles/_variables.scss\#L31-L41

joshuatf

Thanks for all the hard work on this, Maikel! This is a big improvement on our current spacing and how we organize things.

I noticed some oddities around spacing and background color in my environment, but I think this is unrelated to this PR and has to do with Gutenberg styles bleeding through since I have the latest GB enabled. We can address that in a follow-up so we don't block this issue further.

joshuatf · 2023-06-01T17:17:02Z

packages/js/product-editor/src/blocks/section/edit.tsx

+		{
+			className: classNames(
+				'wp-block-woocommerce-product-section__content',
+				`wp-block-woocommerce-product-section__content--block-gap-${ blockGap }`


joshuatf · 2023-06-01T17:18:57Z

packages/js/product-editor/src/components/header/publish-button/publish-button.tsx

-			const noticeContent = isCreating
-				? __( 'Product successfully created.', 'woocommerce' )
-				: __( 'Product published.', 'woocommerce' );
+			const noticeContent = isPublished


Noted this in the other PR, but won't let it block this PR since it's fixed there.

#38507 (comment)

ilyasfoo · 2023-06-16T12:25:37Z

packages/js/customer-effort-score/src/style.scss

 	}
 }

+.components-modal__content {


Hi, @mdperez86! It looks like .components-modal__content is way too general, it's affecting all modals in admin.

Originally reported by Vlad Olaru in this slack p1686918124623299/1686328973.559239-slack-C01SFMVEYAK.

Hi @ilyasfoo, thank you for reporting that.
I created this issue and I'm working on a fix now.

mdperez86 requested a review from a team May 16, 2023 20:23

mdperez86 self-assigned this May 16, 2023

github-actions bot added the package: @woocommerce/customer-effort-score issues related to @woocommerce/customer-effort-score label May 16, 2023

mdperez86 force-pushed the add/38202 branch 3 times, most recently from fa4d6f0 to d65b303 Compare May 23, 2023 15:41

github-actions bot added focus: react admin [team:Ghidorah] package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 23, 2023

mdperez86 force-pushed the add/38202 branch 2 times, most recently from afa3a79 to 0bb738d Compare May 24, 2023 16:03

joshuatf reviewed May 26, 2023

View reviewed changes

mdperez86 added 16 commits May 31, 2023 17:01

Update the margin-left on the buttons in the top bar from 16px to 8px

51a1c39

On mobile, the left and right body margins should be 32 px

3562c0d

The inner padding in the tab in the top bar should be 12px, not 16px

d8df26d

In the edit product link modal, let's set this modal’s width to 650 p…

870c5df

…x and remove the line separating the header from the contents

All modals should now have 8 px window radius

d70bfda

In the feedback form, can we fix the buttons at the bottom

a5c9cce

The help icon in checkboxes small, detached from the item label, and …

17dc02e

…vertically misaligned. Left margin of 4 px, no vertical adjustment; somehow it has a -2px set for the top margin)

Fix block-editor-block-contextual-toolbar top position

702c925

When the product is in a draft state, the button in the top bar shoul…

309a408

…d say Add, not Save. It should change into Update once it's published on the store

Fix layout margin top

e165d75

Add ection support for blockGap and set title and description to opti…

b5c78f5

…onal

Remove non needed control field margin bottom

b481318

Fix schedule sale block spacing

68252d3

Remove non needed control field help text margin bottom

782cbb0

Remove the sub section outer spacing

26b372d

Move pricing fields into a sub section block

02c1203

mdperez86 added 7 commits May 31, 2023 17:01

Fix collapsible-contet margin top

fd32593

Remove invalid inner block layout spacing

4ae4834

Wrap sku, stock toogle and quantity within a sub section

0d04f96

Group inventory advance section inner blocks into a sub section

a9a529f

Fix linter error

4e62bed

Add changelog files

0dd8817

Fix php lint error

ab283d6

mdperez86 force-pushed the add/38202 branch from 0bb738d to ab283d6 Compare May 31, 2023 21:01

Change unit names to fit GB definitions

ab0329f

see: https://github.com/WordPress/gutenberg/blob/b2c16f3c684098a8dabff3911c610caa5c053701/packages/base-styles/_variables.scss\#L31-L41

mdperez86 requested a review from joshuatf May 31, 2023 21:16

joshuatf approved these changes Jun 1, 2023

View reviewed changes

mdperez86 merged commit abcedbe into trunk Jun 1, 2023

mdperez86 deleted the add/38202 branch June 1, 2023 17:56

github-actions bot added this to the 7.9.0 milestone Jun 1, 2023

ismaeldcom mentioned this pull request Jun 15, 2023

Update WCPay welcome page incentives #38689

Merged

ilyasfoo reviewed Jun 16, 2023

View reviewed changes

[MVP QA #1] Visual: spacings #38325

[MVP QA #1] Visual: spacings #38325

Uh oh!

Conversation

mdperez86 commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Uh oh!

github-actions bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuatf May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdperez86 May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mdperez86 commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited

Loading

github-actions bot commented May 16, 2023 •

edited

Loading

joshuatf May 30, 2023 •

edited

Loading

mdperez86 May 31, 2023 •

edited

Loading