-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Sharing: fix connecting conflicting accounts #12707
Conversation
Test live: https://calypso.live/?branch=fix/fb-connect |
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.
Nice find! Testing works 👍
Thanks @jhnstn since this bug is rather severe I'm going to go ahead and 🚢 this. We can iterate on the warning message in another PR if folks would like changes |
status="is-warning" | ||
icon="notice" | ||
text={ this.props.translate( 'The marked connection will be replaced with your selection.' ) } | ||
isCompact /> |
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.
Would we have needed to apply showDismiss={ false }
since the default is true and previously we'd disabled it? Or are styles hiding the dismiss?
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.
Looking at https://wpcalypso.wordpress.com/devdocs/design/notices I don't believe compact notices have a dismiss option, I could add it if it's more clear
This PR fixes an issue where clicking on "Add a different account" will crash the page when adding an additional account that has a conflict. Eg, we've added a Facebook account, but we want to replace add a Facebook Page that's already linked to the account.
Testing Instructions
In prod this will hard crash your browser, since we get into an infinite loop.
Notes
It appears that a dispatch to display a warning notice in render was causing this to explode. As an alternative I opted for an inline notice. Design suggestions are welcome. @obenland suggested that maybe we should just update the instruction text in this case?