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.
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
andContactInformation
components for contact info #917Implement
PhoneNumberCard
andContactInformation
components for contact info #917Changes from 7 commits
05ba64b
5ba451b
5de074b
18cd8fa
17d5581
71ef39f
2deed28
8f42c78
2a59a99
6e839d8
2a5ab61
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Sorry, I'm not quite sure of your meaning. Could you provide some code examples?
The logic is used for rendering different visual results.
The
<Spinner>
is shown in the first, and the<AppSpinner>
is for the second one.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 thoughtApp-
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