Add support phone and email in the admin settings page #5471
Conversation
|
Size Change: +69.7 kB (+6%) 🔍 Total Size: 1.29 MB
ℹ️ View Unchanged
|
htdat
force-pushed
the
add/3981-support-phone-number
branch
from
1119ef0 to
30b511d
Compare
htdat
force-pushed
the
add/3981-support-phone-number
branch
from
30b511d to
0b69c12
Compare
htdat
changed the title
|
Out of curiosity, is there an opportunity to use TypeScript for new files? Context: paJDYF-6MV-p2 |
@kalessil - I tried to use TypeScript but the new files use some non-TypeScript functions such as |
Gotcha, thanks for sharing the details! |
There was a problem hiding this comment.
Great job @htdat , code looks clean and excellent, and also tests well! 👏
Not marking this as a request for changes, as I'm not certain they're needed, but I have a couple things to discuss.
One weird issue I encountered was not being able to save settings at all initially. After digging a bit, it turned out that Stripe rejects my valid phone number as invalid. Afterwards the server return an exception, however the exception/error is not bubbled up from WC_REST_Payments_Settings_Controller::update_account, and I see a success pill, stating settings were saved correctly. This is an issue, but not one for this PR.
- When they have never been set.
I've debugged a bit and it seems that I will debug a bit more and add a new issue for this. @RadoslavGeorgiev - I've addressed other concerns in your inline comments too. Ready to review again. |
There was a problem hiding this comment.
I will debug a bit more and add a new issue for this.
Thanks for digging into this! I'm not sure if it's applicable in contexts outside of the new fields, but in any case it would be nice to have another issue to keep track of it! 🤝
The rest of the PR looks excellent, there's just one detail that remains to be fixed :)
| @@ -80,6 +84,9 @@ const Transactions = () => { | |||
| <span className="input-help-text" aria-hidden="true"> | |||
| { `${ accountStatementDescriptor.length } / ${ ACCOUNT_STATEMENT_MAX_LENGTH }` } | |||
| </span> | |||
|
|
|||
| <SupportEmailInput onErrorMessage={ setSaveDisabled } /> | |||
| <SupportPhoneInput onErrorMessage={ setSaveDisabled } /> | |||
There was a problem hiding this comment.
The state of both inputs needs to be stored separately in order to determine whether saving is disabled or not. Right now:
- I opened the page with valid values in both fields.
- Deleted the email ➡️ The save button got disabled.
- Deleted the phone number ➡️ The save button remain disabled.
- Started typing an email (which is not validated) ➡️ The save button immediately got enabled, even though the phone number remains empty.
@RadoslavGeorgiev - ready for review again. |
Added this issue #5516 |
!
There was a problem hiding this comment.
@htdat is is possible to load the <PhoneNumberInput component in a lazy fashion so that it doesn't effect the load times of the page?
|
@naman03malhotra - as discussed with you via DM. I am summarizing the discussion here. I don’t see much slower in the page load time. In fact, the new package adds up the size of Since it was validated to use in two other places (below), I think it’s generally fine. What I am doing here is to just abstract the existing component so that it can be reused. |
Fixes #3981
Changes proposed in this Pull Request
SupportEmailInputandSupportPhoneInputfromclient/card-readers/settings/sections/contacts-details.js.Card Readerssetting page and WCPay general settings pages.Testing instructions
npm start.If possible, try to test with
Card Readerssettings page:trueforhas_card_readers_availableflag/method, like this 2ef2d-pbCard Readers> Receipt Details.Note 1
GitHub action throws a SOS warning about increasing sizes of JS and CSS for the admin settings page. I think this is fine as it brings a nice UI and validation for the phone input. Under the hood, it uses the
PhoneNumberInputcomponent, which is based on 'intl-tel-input' package.Note 2
With the connected account, there is no option for this. Stripe always requires this number and here is the most detail I can get when trying to update this number via
curl, Stripe Dashboard, and via API call through local server:npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge