Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Create shipping fee field block and initial shipping section #37642

Merged
merged 7 commits into from Apr 15, 2023
Merged

Create shipping fee field block and initial shipping section #37642

merged 7 commits into from Apr 15, 2023

Conversation

mdperez86
Copy link
Contributor

@mdperez86 mdperez86 commented Apr 10, 2023 •
edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes #37406

How to test the changes in this Pull Request:

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

  1. Got to /wp-admin/tools.php?page=woocommerce-admin-test-helper
  2. Under Features tab make sure to enable block-editor-feature-enabled
  3. Then visit /wp-admin/admin.php?page=wc-admin&path=/add-product
  4. Under the Shipping tab in the Fees & dimensions section the shipping fee fields should be shown
Screen.Recording.2023-04-10.at.2.21.34.PM.mov

@mdperez86 mdperez86 requested a review from a team April 10, 2023 18:19
@mdperez86 mdperez86 self-assigned this Apr 10, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2023 •
edited

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 github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 10, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 10, 2023 •
edited

Test Results Summary

Commit SHA: 118cda4

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 52s
E2E Tests1850010019514m 3s
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.

@mdperez86 mdperez86 force-pushed the add/37406 branch 2 times, most recently from 734d855 to 44b99a6 Compare April 11, 2023 19:43
@codecov
Copy link

codecov bot commented Apr 11, 2023 •
edited

Codecov Report

Merging #37642 (118cda4) into trunk (dfdc2d3) will decrease coverage by 0.0%.
The diff coverage is 0.0%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #37642     +/-   ##
==========================================
- Coverage     51.6%    51.6%   -0.0%     
  Complexity   17260    17260             
==========================================
  Files          429      429             
  Lines        79863    79939     +76     
==========================================
+ Hits         41211    41212      +1     
- Misses       38652    38727     +75
Impacted Files Coverage Δ
...ugins/woocommerce/includes/class-wc-post-types.php 1.1% <0.0%> (-0.1%) ⬇️
...ludes/tracks/events/class-wc-products-tracking.php 0.0% <ø> (ø)

... and 1 file with indirect coverage changes

joshuatf
joshuatf reviewed Apr 12, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and no issues with testing! Left one comment about reusing the radio control block component, but otherwise LGTM.

packages/js/product-editor/src/blocks/shipping-fee/edit.tsx Outdated
<div className="wp-block-column">
<RadioControl
label={
<span className="wp-block-woocommerce-product-radio__title">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we reduce the possibility of introducing breaking changes in the future, could we use the actual block radio component here?

The pattern I've seen in GB and WC Blocks is to extract this portion into a separate block.tsx file so that we can reuse in both the radio control's Edit component and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to suggest to not reuse a part of another block if that part is within the same block folder. I think blocks should be reused only via templates and inner blocks. If we want to have shared parts then we should create components outside of each block that are gonna be using the shared parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done -> ae58d28

packages/js/product-editor/src/blocks/shipping-fee/edit.tsx Outdated
const { title } = attributes;

const blockProps = useBlockProps( {
className: 'wp-block-woocommerce-product-shipping-fee-fields',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as other PR regarding manually overriding class names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see -> #37683 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responded here. Please let me know if I misunderstood or if there's a different reason we need to add the class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done -> 0036e1e

@mdperez86 mdperez86 requested a review from joshuatf April 12, 2023 20:43
@mdperez86 mdperez86 force-pushed the add/37406 branch 2 times, most recently from 0036e1e to bf35856 Compare April 14, 2023 17:30
joshuatf
joshuatf previously approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice work 🎉

@mdperez86 mdperez86 requested a review from joshuatf April 14, 2023 18:13
joshuatf
joshuatf approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mdperez86 mdperez86 merged commit 42cc482 into trunk Apr 15, 2023
19 of 21 checks passed
@mdperez86 mdperez86 deleted the add/37406 branch April 15, 2023 02:44
@github-actions github-actions bot added this to the 7.7.0 milestone Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuatf joshuatf joshuatf approved these changes

Assignees

@mdperez86 mdperez86

Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Milestone
7.7.0
Development

Successfully merging this pull request may close these issues.

Create shipping fee field block and initial shipping section
2 participants
@mdperez86 @joshuatf