-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
Implement PhoneNumberCard
and ContactInformation
components for contact info
#917
Conversation
4df92ad
to
2deed28
Compare
Hopefully, it won't get taken down again or messed around due to some controversies, https://medium.com/@catamphetamine/how-github-blocked-me-and-all-my-libraries-c32c61f061d3.
😍 Thanks |
Speaking of potential problems with dependencies, do we have any policy of evaluating the dependencies we use? Especially, that here, we feed out customers' phone numbers to external software. BTW, 📜 I don't think we need to address this here, or is it a strict blocking issue. We already process our code through an endless amount of don't-know-how-well-audited dependencies - Webpack & a handful of plugins. |
if ( loaded ) { | ||
phoneNumberContent = ( | ||
<> | ||
<CardDivider /> | ||
<PhoneNumberContent | ||
initCountry={ data.country } | ||
initNationalNumber={ data.nationalNumber } | ||
onPhoneNumberChange={ onPhoneNumberChange } | ||
/> | ||
</> | ||
); | ||
} else { | ||
phoneNumberContent = <AppSpinner />; | ||
} | ||
} else { | ||
description = loaded ? data.display : <Spinner />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Having the preloading logic here and in https://github.com/woocommerce/google-listings-and-ads/pull/917/files#diff-51ff763be0b20871d193755311c5c437caafd6c4d46bb1413de599e1124a7714R53 looks like too much.
I think we can make containing element render this component. Then, this component could decide whether to render a scaffold with a tiny preloader in place of a phone number, or a fully prepared input control.
Also, in one case we use <Spinner>
in the other <AppSpinner>
. Is that on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I put the wrong link.
My main point was about handling loaded here and in ContactInformation
https://github.com/woocommerce/google-listings-and-ads/pull/917/files#diff-51ff763be0b20871d193755311c5c437caafd6c4d46bb1413de599e1124a7714R53
what results in
Here is code sample/suggestion https://github.com/woocommerce/google-listings-and-ads/pull/917/files#r681624815
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of <Spinner>
vs. <AppSpinner>
now I got it, thanks. I agree we need both here. I got mislead by their names. I thought App-
one is just profiled for our need, while they are in fact <InlineSpinenr>
and <BlockSpinner>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 📜 For the block spinner <AppSpinner>
, I'd adapt its position/margin/padding to make the card fixed height = do not jump once loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the loading UI in the later PR #921, and it shows a spinner with a card layout instead. The demo in another PR shows that change:
Kapture.2021-08-02.at.18.37.16.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, reviewed the code.
Works fine, but left few comments regarding the code improvements.
…ove `as` parameter Address #917 (comment)
{ phone.loaded ? ( | ||
<VerticalGapLayout size="large"> | ||
<PhoneNumberCard | ||
phoneNumber={ phone } | ||
initEditing={ initEditing } | ||
onPhoneNumberChange={ onPhoneNumberChange } | ||
/> | ||
<div>TODO: add store address card</div> | ||
</VerticalGapLayout> | ||
) : ( | ||
<AppSpinner /> | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of #917 (comment) I think this loading state handling is redundant, and delivers slightly worse UX.
We can remove the redundancy from ContactInformation
and let the PhoneNumberCard
handle loading state itself.
{ phone.loaded ? ( | |
<VerticalGapLayout size="large"> | |
<PhoneNumberCard | |
phoneNumber={ phone } | |
initEditing={ initEditing } | |
onPhoneNumberChange={ onPhoneNumberChange } | |
/> | |
<div>TODO: add store address card</div> | |
</VerticalGapLayout> | |
) : ( | |
<AppSpinner /> | |
) } | |
<VerticalGapLayout size="large"> | |
<PhoneNumberCard | |
phoneNumber={ phone } | |
initEditing={ initEditing } | |
onPhoneNumberChange={ onPhoneNumberChange } | |
/> | |
<div>TODO: add store address card</div> | |
</VerticalGapLayout> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the change suggested above, we would have to change initEditing
handling.
Personally, in ContactInformation
I'd cater for isSetupMC
only. Then let PhoneNumberCard
reason about phone.data.isValid
+ some given property.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare this way to #917 (comment), it could only deliver the phone icon and "Phone number" title earlier about 0.5 ~ 1.5 seconds at the most. The rest elements are still needed to wait for data loaded. I'm not sure if that is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say 0.5~1.5 s on a developer machine is a lot. Then imagine how long it could take on a low-end device with a slow network.
I'm not sure if that is worth it.
Personally, I see it the opposite, compare:
- Deliver a scaffold instantly, with less code, and a single component that's responsible for the preloading state of its data
- Deliver a blank space (or card), with more code, and preloading logic and responsibility scattered around 2 components (3 in Add contact information section and its edit subpage to the settings page #921)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened another PR #934 to change the handling of loading states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, delivers the needed feature. I think it's good to merge to unlock the following PRs, as we would touch on preloading changes later in #921.
We can get back to the discussion from #917 (comment) there or in a separate issue 📜 hopefully soon, until we build too much on top of that, making it hard to change.
I'd also appreciate @jconroy take on evaluating npm dependencies and policy for sending PII to external libraries.
Changes proposed in this Pull Request:
Partial of #863 and based on #914
libphonenumber-js
to dependencies💡 Please note that this PR doesn't include the API integration.
Screenshots:
🎥 . When the phone number got from Google MC is empty
Kapture.2021-07-29.at.18.17.36.mp4
🎥 . When the phone number got from Google MC has already exist
Kapture.2021-07-29.at.18.15.50.mp4
📷 . The preview mode of PhoneNumberCard will be used in the Setting page later
Detailed test instructions:
For easier testing, there is a mock phone number that is 50% chance for an empty phone number and 50% for an existed phone number when every time reload the page.
google-listings-and-ads/js/src/data/selectors.js
Lines 71 to 78 in 4df92ad
Changelog entry