-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
UX: Allow quick Add Account name based on default name #20168
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
@@ -43,7 +43,7 @@ export const CreateAccount = ({ onActionComplete }) => { | |||
const { isValidAccountName, errorMessage } = getAccountNameErrorMessage( | |||
accounts, | |||
{ t }, | |||
newAccountName, | |||
newAccountName === '' ? defaultAccountName : newAccountName, |
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.
This could be shortened and clarified to:
newAccountName ?? defaultAccountName
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.
This ended up nullifying the change. The ??
operator only checks for null and undefined, but the newAccountName
is always an empty string if it's unset (never nullish). So the default account name is never used here, leaving the behavior of the component exactly as it was before.
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.
Oh my! Should I fix this? I guess it should be ||
instead of ??
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.
Yeah exactly! ||
should work well here. A fix would be appreciated.
We found this during RC testing but this improvement wasn't urgent, so we removed it from the RC. That is, fixing this isn't urgent, so no rush.
Builds ready [12fc72f]
Page Load Metrics (1577 ± 35 ms)
Bundle size diffs
|
Codecov Report
@@ Coverage Diff @@
## develop #20168 +/- ##
========================================
Coverage 69.44% 69.44%
========================================
Files 986 986
Lines 37291 37293 +2
Branches 10014 10015 +1
========================================
+ Hits 25895 25897 +2
Misses 11396 11396
|
Builds ready [9c6e8db]
Page Load Metrics (1524 ± 39 ms)
Bundle size diffs
|
Explanation
Formerly it was possible to quickly create a new account with the next default account name but this was recently changed. If the new account input is empty, the placeholder value should be used as the account name.
Screenshots/Screencaps
Screen.Recording.2023-07-24.at.9.32.37.PM.mov
Manual Testing Steps
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Board
label.In this case, a QA Engineer approval will be be required.