Skip to content
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

Merged
merged 2 commits into from
Apr 3, 2017
Merged

Sharing: fix connecting conflicting accounts #12707

merged 2 commits into from
Apr 3, 2017

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Mar 31, 2017

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

  • Navigate to http://calypso.localhost:3000/sharing/:siteId:
  • Connect an account that has a facebook page, but select the main account
  • Connect another facebook account, a dialog should appear that lets you select either the page, but with a notice that it will replace the existing connection
  • Clicking proceeds through connection flow
  • You can connect other social accounts

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?

screen shot 2017-03-31 at 2 26 43 pm

@gwwar gwwar added [Feature] Sharing Features and settings for sharing posts across different platforms, including sharing buttons. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 31, 2017
@gwwar gwwar self-assigned this Mar 31, 2017
@gwwar gwwar requested review from jhnstn, obenland, artpi and lamosty March 31, 2017 21:48
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Mar 31, 2017
@gwwar gwwar requested a review from aduth March 31, 2017 21:50
@gwwar
Copy link
Contributor Author

gwwar commented Mar 31, 2017

cc @jjchrisdiehl

Copy link
Member

@jhnstn jhnstn left a 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 👍

@gwwar
Copy link
Contributor Author

gwwar commented Apr 3, 2017

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

@gwwar gwwar merged commit 321237d into master Apr 3, 2017
@gwwar gwwar deleted the fix/fb-connect branch April 3, 2017 17:11
@matticbot matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 3, 2017
status="is-warning"
icon="notice"
text={ this.props.translate( 'The marked connection will be replaced with your selection.' ) }
isCompact />
Copy link
Contributor

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?

Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sharing Features and settings for sharing posts across different platforms, including sharing buttons. [Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants