Separate Phone and Address settings #1025

tomalec · 2021-09-24T19:06:49Z

Changes proposed in this Pull Request:

Add placeholder mixin, (659d29d)
to make it easier to add preloaders more subtle than spinners.
(Copied the code from woocommerce/woocommerce-admin@9f68059/packages/style-build/abstracts/_mixins.scss#L22-L35)
Add support for custom appearance to AccountCard (a7286c2)
So we could provide any title when needed.
Split EditContactInformation, (568bb3e, e836e5e)
into EditPhoneNumber and EditStoreAddress.
Separate preview mode of StoreAddressCard and PhoneNumberCard into separate components. (c21c74c, bc3b635, 55f63d0, 8f349e7)
Extract shared code into ContactInformationPreviewCard.
Simplify the logic. Change "Edit" to internal link.
This PR changes "Edit" buttons to be regular links, so they could be ctrl + clicked, and navigated with browser built-in features.

Addresses #962 (comment)

Screenshots:

Detailed test instructions:

Happy path

Go to the settings page /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
Wait for the phone number and address to load correctly
Check that the page looks like in the designs
Click the "Edit" button by the "Phone number"
Go back to 1., Click the "Edit" button by the "Store address"

Missing contact info.

This is quite hard to achieve with newly created accounts. Ideally, you should have an old account w/o a store/phone address.

Different store address

Go to Woo settings /wp-admin/admin.php?page=wc-settings
Change the address to something else than the current one.
Go to the settings page /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
You should eventually see the warning about the missing store address.

Mocked missing phone number

Change /js/src/data/actions.js#L521 to data: { ...data, phone_number: '' },`.
npm run dev
Go to the settings page /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
You should eventually see the warning about the missing phone number.

Changelog entry

Split contact information settings page to phone and address settings.

Additional notes

<ContactInformationPreviewCard> sets aria-busy only on the preloader, I think it should be set on the entire card, but I'm not sure how to achieve it given we don't have the reference to the Card's container DOM element, and React does not expose something like the native ElementInternals.

To be done later, in separate PRs

Adapt Edit Store Address and Edit Phone Number pages.
Add e2e tests, once we make them work past account connection.

to make it easier to add preloaders more subtle than spinners. Copy the code from https://github.com/woocommerce/woocommerce-admin/blob/9f68059c7df466ee7a559c555bcb5a7b9e421e6d/packages/style-build/abstracts/_mixins.scss#L22-L35

into `EditPhoneNumber` and `EditStoreAddress`. Addresses #962 (comment)

to simplify the logic, change "Edit" to internal link.

So we could provide any title when needed.

Open in the same tab.

when the data is missing. So they could show links to separated pages. Adjust `StoreAddressCardPreview` to show profiled warning.

To be shared with `PhoneNumberCard` preview.

as described in #962 (comment).

Show a warning when the address is different than the WC one.

js/src/components/contact-information/contact-information-preview-card.js

eason9487 · 2021-09-28T08:08:18Z

js/src/components/contact-information/contact-information-preview-card.js

+			isSecondary
+			iconSize={ 16 }
+			iconPosition="right"
+			href={ editHref }


💅
It provides better accessibility but makes browser load the next page from the HTML document. Maybe it could take the same way as the "View Reports" button.

google-listings-and-ads/js/src/dashboard/index.js

Lines 40 to 42 in 09f25d8

<Link href={ getNewPath( null, '/google/reports' ) }>

<Button isPrimary>View Reports</Button>

</Link>

Thanks for catching this!

It seems to be linked to a more generic problem. Another one in the batch of WC vs WP Links.
I created an issue in WC-admin repo.

To me <Link><Button> still has a serious A11Y issue, with being seen as "Edit - button" instead of "Edit - visited link".
I'll create an issue in our repo as well, to solve it somehow for all internal button links.
#1027

<Link>Button> -> <a><button> is also an invalid HTML, so I'd leave it as is, and try to fix it sooner than later in #1027, by cloning wc-admins wcAdminLinkHandler to AppButton.

try to fix it sooner than later in #1027, by cloning wc-admins wcAdminLinkHandler to AppButton.

Thanks! I also think it's feasible.

js/src/components/contact-information/contact-information-preview-card.js

js/src/components/contact-information/phone-number-card/phone-number-card-preview.js

js/src/components/contact-information/store-address-card.js

eason9487 · 2021-09-28T09:57:35Z

js/src/utils/urls.js

@@ -10,7 +10,8 @@ export const subpaths = {
 	editFreeListings: '/free-listings/edit',
 	editCampaign: '/campaigns/edit',
 	createCampaign: '/campaigns/create',
-	editContactInformation: '/edit-contact-information',


Sorry🙏 , I forgot to list a corresponding URL reference. It needs to change as well.

google-listings-and-ads/src/PluginHelper.php

Lines 147 to 149 in 09f25d8

protected function get_contact_information_setup_url(): string {

return admin_url( 'admin.php?page=wc-admin&path=/google/settings&subpath=/edit-contact-information' );

}

Ah, sorry, I got it on my paper to do list, but forgot about it.

I think, the best we can do now, given, there is no single contact information page, is to point merchants to the overall settings page.

Or do you think we should have two separate notes now?

I think, the best we can do now, given, there is no single contact information page, is to point merchants to the overall settings page.

I was thinking about it in the same way.

Or do you think we should have two separate notes now?

I think this would be adjusted later. According to Figma, after the phone verification state being available from Google end, there will be a new notification box and dedicated account issue that point to the edit phone number subpage in Settings.

eason9487 · 2021-09-28T10:00:23Z

js/src/components/contact-information/contact-information-preview-card.js

+			href={ editHref }
+			text={ __( 'Edit', 'google-listings-and-ads' ) }
+			eventName={ editEventName }
+			eventProps={ { path: getPath(), subpath } }


📝 +❔
<ContactInformationPreviewCard> is only used in the main settings page after this PR. The subpath is always undefined here. And it also depends on whether the relevant #1025 (comment) has further adjustment. Maybe there's no need to have this tracking event prop.

My mental model was that ContactInformationPreviewCard is a component, which means it should not impose, or be aware apriori, where/at what URL it will be used.

Therefore it should assume it could be used anywhere on a shallow or deep page.

BTW, I was thinking that maybe we should add this path, [subpath] props on a generic level to all props, as we already have it or context on the majority of events.
IMHO, it's quite easy to get and gives quite detailed information to a tracks consumer.

My mental model was that ContactInformationPreviewCard is a component, which means it should not impose, or be aware apriori, where/at what URL it will be used.

Therefore it should assume it could be used anywhere on a shallow or deep page.

Got it.

What I thought before was that it might always be undefined under the use cases we known and if there are any further adjustments from the #1025 (comment). So it could be a bit strange to just look at the code if the "always be undefined" happened.

Considering that a component shouldn't be imposed where it will be used, I thought there's a common way that merges a react prop eventProps given from consumer sides.

But we can get the clue of subpath from the tracking README and have the subsequence adjustments - 54aea65 and 1db2271. I think it should be ok for now.

Also, if we would have solved routing in a sense that path could be as well /settings, /settings/store-address, /settings/store-address/coordinates, without the need of separate query param for every level, the problem would be solved without a need of picking those props in all the places.

I made a proposal/POC/RFC at #1031

as suggested in #1025 (comment).

Co-authored-by: Eason <[email protected]>

…e/google-listings-and-ads into feature/962-separate-settings

addresses #1025 (comment).

Addresses #1025 (comment).

as now there is no single contact information page. Addresses #1025 (comment).

tomalec · 2021-09-29T18:20:43Z

@eason9487 I addressed or commented on all your comments, could you take this PR for another round?

Add the actual code and tracking readme changes, as the changes made before in 54aea65 changed only JSDoc. Addresses #1025 (comment).

eason9487

Tested locally and works well. LGTM.

tomalec added 6 commits September 24, 2021 18:16

Add placeholder mixin,

659d29d

to make it easier to add preloaders more subtle than spinners. Copy the code from https://github.com/woocommerce/woocommerce-admin/blob/9f68059c7df466ee7a559c555bcb5a7b9e421e6d/packages/style-build/abstracts/_mixins.scss#L22-L35

Split EditContactInformation,

e836e5e

into `EditPhoneNumber` and `EditStoreAddress`. Addresses #962 (comment)

Separate StoreAddressCardPreview component,

c21c74c

to simplify the logic, change "Edit" to internal link.

Add support for custom appearance to AccountCard

a7286c2

So we could provide any title when needed.

Make StoreAddressCardPreview Edit link internal.

8f349e7

Open in the same tab.

Let each contact information card show respective warning,

568bb3e

when the data is missing. So they could show links to separated pages. Adjust `StoreAddressCardPreview` to show profiled warning.

tomalec mentioned this pull request Sep 24, 2021

Submit business contact information - full version #962

Closed

17 tasks

tomalec added 3 commits September 27, 2021 15:50

Extract non address specific parts from StoreAddressCard.

bc3b635

To be shared with `PhoneNumberCard` preview.

Adapt PhoneNumberCard preview to the splited settings,

55f63d0

as described in #962 (comment).

Render the MC address data on settings page.

cb93bce

Show a warning when the address is different than the WC one.

tomalec marked this pull request as ready for review September 27, 2021 16:25

tomalec self-assigned this Sep 27, 2021

tomalec requested a review from a team September 27, 2021 16:25

tomalec changed the title ~~WIP: Separate Phone and Address settings~~ Separate Phone and Address settings Sep 27, 2021

tomalec added 2 commits September 27, 2021 18:28

Remove unused getEditContactInformationUrl.

a308d01

Add edit_mc_phone_number to Tracking README.

09f25d8

Base automatically changed from feature/962-integrate-phone-verifi-api to develop September 28, 2021 03:28

eason9487 reviewed Sep 28, 2021

View reviewed changes

This was referenced Mar 19, 2022

Support @wordpress/components.Button isLink as wc-admin link woocommerce/woocommerce#32231

Closed

Impaired UX/A11Y of internal button links. #1027

Open

Remove not needed props from ContactInformationPreviewCard

df5da41

as suggested in #1025 (comment).

tomalec added the type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies label Sep 28, 2021

tomalec and others added 6 commits September 28, 2021 22:12

Fix typos and obsolete code comments.

797a566

Co-authored-by: Eason <[email protected]>

Merge branch 'feature/962-separate-settings' of github.com:woocommerc…

1ed49d4

…e/google-listings-and-ads into feature/962-separate-settings

Update JSDoc for ContactInformationPreviewCard,

79721a9

addresses #1025 (comment).

Fix the default value of ContactInformationPreviewCard's description.

ab4f133

Addresses #1025 (comment).

Split edit_mc… and edit_wc_store_address tracking events.

54aea65

Addresses #1025 (comment).

Make "add contact information" noted point to Settings Page URL,

a4f0f20

as now there is no single contact information page. Addresses #1025 (comment).

tomalec requested a review from eason9487 September 29, 2021 18:22

Split edit_mc… and edit_wc_store_address tracking events.

1db2271

Add the actual code and tracking readme changes, as the changes made before in 54aea65 changed only JSDoc. Addresses #1025 (comment).

tomalec mentioned this pull request Sep 29, 2021

Separate settings view mode from ContactInformation #1029

Merged

eason9487 approved these changes Sep 30, 2021

View reviewed changes

tomalec mentioned this pull request Sep 30, 2021

RFC: Remove subpath URLQueryParam from settings pages. #1031

Open

3 tasks

tomalec merged commit 51c825e into develop Sep 30, 2021

tomalec deleted the feature/962-separate-settings branch September 30, 2021 13:31

eason9487 added this to the 1.5 milestone Oct 1, 2021

eason9487 mentioned this pull request Nov 13, 2024

Consolidate accounts onboarding: Merge the store address setting in Step 3 into Step 1 and remove Step 3 along with the phone verification #2653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Phone and Address settings #1025

Separate Phone and Address settings #1025

tomalec commented Sep 24, 2021 •

edited

Loading

eason9487 Sep 28, 2021

tomalec Sep 28, 2021 •

edited

Loading

tomalec Sep 28, 2021

eason9487 Sep 29, 2021

eason9487 Sep 28, 2021

tomalec Sep 29, 2021

tomalec Sep 29, 2021

eason9487 Sep 30, 2021

eason9487 Sep 28, 2021

tomalec Sep 29, 2021

eason9487 Sep 30, 2021

tomalec Sep 30, 2021

tomalec Sep 30, 2021

tomalec commented Sep 29, 2021

eason9487 left a comment

	<Link href={ getNewPath( null, '/google/reports' ) }>
	<Button isPrimary>View Reports</Button>
	</Link>

	protected function get_contact_information_setup_url(): string {
	return admin_url( 'admin.php?page=wc-admin&path=/google/settings&subpath=/edit-contact-information' );
	}

Separate Phone and Address settings #1025

Separate Phone and Address settings #1025

Conversation

tomalec commented Sep 24, 2021 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Happy path

Missing contact info.

Different store address

Mocked missing phone number

Changelog entry

Additional notes

To be done later, in separate PRs

Choose a reason for hiding this comment

tomalec Sep 28, 2021 • edited Loading

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

tomalec commented Sep 29, 2021

eason9487 left a comment

Choose a reason for hiding this comment

tomalec commented Sep 24, 2021 •

edited

Loading

tomalec Sep 28, 2021 •

edited

Loading