This repository was archived by the owner on Feb 23, 2024. It is now read-only.
Move phone to default fields section instead of being handled inline. #11651
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
senadir
added
block: checkout
woocommercebot
requested review from
a team and
nielslange
and removed request for
a team
senadir
force-pushed
the
refactor/move-phone-to-default-address
branch
from
dbf6bdc to
3e944c8
Compare
Contributor
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
senadir
force-pushed
the
refactor/move-phone-to-default-address
branch
from
3e944c8 to
6fddfa4
Compare
senadir
commented
Nov 7, 2023
| showShippingMethods: needsShipping && ! prefersCollection, | ||
| showBillingFields: | ||
| ! needsShipping || ! useShippingAsBilling || prefersCollection, | ||
| ! needsShipping || ! useShippingAsBilling || !! prefersCollection, |
Member
Author
There was a problem hiding this comment.
TS fix. prefersCollection can be either a boolean or undefined, this accounts for and assumes undefined is false so that showBillingFields and useBillingAsShipping are always boolean.
senadir
commented
Nov 7, 2023
| value={ billingAddress.phone } | ||
| onChange={ ( value ) => { | ||
| setBillingPhone( value ); | ||
| dispatchCheckoutEvent( 'set-phone-number', { |
Member
Author
There was a problem hiding this comment.
It was not clear why those events were fired by their own (maybe because editing the field didn't trigger the parent set-shipping-address|set-billing-address hook). This is now deleted. It was experimental.
Contributor
|
Size Change: -659 B (0%) Total Size: 1.56 MB
ℹ️ View Unchanged
|
senadir
commented
Nov 7, 2023
Comment on lines
-55
to
-61
| addAction( | ||
| `${ actionPrefix }-checkout-set-phone-number`, | ||
| namespace, | ||
| ( { step, ...rest }: { step: string; storeCart: StoreCart } ): void => { | ||
| trackCheckoutStep( step === 'shipping' ? 2 : 3 )( rest ); | ||
| } | ||
| ); |
Member
Author
There was a problem hiding this comment.
This was meant to track at which step we're right now, other events will continue to do so without needing this.
senadir
commented
Nov 7, 2023
| label: __( 'Phone', 'woo-gutenberg-products-block' ), | ||
| optionalLabel: __( 'Phone (optional)', 'woo-gutenberg-products-block' ), | ||
| autocomplete: 'tel', | ||
| type: 'tel', |
Member
Author
There was a problem hiding this comment.
Newly introduced type field.
senadir
commented
Nov 7, 2023
| ariaDescribedBy={ describedBy } | ||
| value={ value } | ||
| title="" | ||
| title="" // This prevents the same error being shown on hover. |
Member
Author
There was a problem hiding this comment.
This was only mentioned in the original PR #8143 (comment), it's better near the code now.
nielslange
reviewed
Nov 8, 2023
Contributor
nielslange
left a comment
nielslange
left a comment
There was a problem hiding this comment.
Thanks for working on this, @senadir.
While testing, I noticed that testing this PR is affected by #11583, as toggling the phone field causes this error:
I also noticed that the phone field appears twice on the Checkout block. This affects both the shipping and billing address. This issue is only visible in the frontend, and cannot be reproduced consistently.
Page editor:
|
Frontend:
|
Member
Author
|
Thank you! I will try to reproduce why you're seeing the phone field twice. |
Contributor
@senadir After deleting the Checkout block and adding it again, I could no longer see this issue. I wonder if the issue was related to me testing the PR with an existing Checkout block. 🤔 |
Member
Author
|
Other than #11583, are you seeing any other issues? |
Contributor
Nope. The rest is working fine. |
mikejolley
reviewed
Nov 8, 2023
Member
mikejolley
left a comment
mikejolley
left a comment
There was a problem hiding this comment.
Noticed 2 issues testing this but they might be unrelated/semi-related.
- Toggling phone off then on in the editor causes block error.
- The phone is showed in the condensed address widget when phone is off.
Member
Author
|
Thank you @mikejolley, I understand that both of those issues should be fixed after merging #11714 |
senadir
force-pushed
the
refactor/move-phone-to-default-address
branch
from
98cd233 to
a6a7f31
Compare
senadir
commented
Nov 14, 2023
| ) ) } | ||
| </div> | ||
| { address.phone && showPhoneField ? ( | ||
| { address.phone && ! fieldConfig.phone.hidden ? ( |
Member
Author
There was a problem hiding this comment.
We now use the fieldConfig setup instead of passing down props.
senadir
commented
Nov 14, 2023
| { [ | ||
| address.address_1, | ||
| address.address_2, | ||
| ! fieldConfig.address_2.hidden && address.address_2, |
Member
Author
There was a problem hiding this comment.
I also went along and fixed address_2 so it's not visible if the field is not visible.
Member
Author
|
@mikejolley @nielslange I rebased this PR and would appreciate another review. |
mikejolley
approved these changes
Nov 14, 2023
Member
mikejolley
left a comment
mikejolley
left a comment
There was a problem hiding this comment.
I tested your scenarios and placed orders with phone enabled/disabled. Seems to be working as intended. If CI passes you can ship it.
senadir
force-pushed
the
refactor/move-phone-to-default-address
branch
from
a6a7f31 to
6c58eeb
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the shipping and billing phones to be in the default address sections instead of being handled inline, and removes a class of issues related to that, as well as reduce complexity of codebase.
Something like #10602 since we always handled this on our own in the code.
This also fixes an issue in which phone field was always semantically outside the other fields div, and would sometimes render as a full line despite there being space for it to be half line above.
The one change is that
set-billing-phoneandset-shipping-phoneevents are no longer fired,set-shipping-addresssandset-billing-addresswill fire on phone is changed, this aligns it with the rest of fields in that form.Also right now, using locale filters, someone can reposition a phone number if they want.
This is a soft prerequisite to the custom fields project.
Testing Instructions
Do not include in the Testing Notes
Should be tested by the development team exclusively
WooCommerce Core
Feature plugin
Experimental
N/A
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog