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.

Sign up for GitHub

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

Jump to bottom

Sharing: Refactor sharing buttons to use Redux instead of Flux Stores #10278

Merged
merged 1 commit into from
Dec 30, 2016
Merged

Sharing: Refactor sharing buttons to use Redux instead of Flux Stores #10278

merged 1 commit into from
Dec 30, 2016

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Dec 27, 2016 •
edited by artpi
Loading

In this PR I'm refactoring the Sharing Buttons Component to use Redux Actions instead of Flux Stores. This PR includes:

  • Using site-settings redux subtree
  • Using sharing-buttons redux subtree
  • Using post-types redux subtree
  • Dropping the sharing buttons list Flux Store
  • Refactor the components using the last coding standards (No mixins, no analytics lib, no notices lib)
  • Avoid using the noticesmiddleware for SiteSettings notices, this would have the effect of triggering multiple notices for the same "save". So, I moved this behaviour to the component instead.

One nice to have enhancement (that could be done in another PR) is to avoid using the site Prop. This would require refactoring the underlying components.

Testing instructions

  • Open the /sharing/buttons/$site
  • Try to edit and save the sharing buttons settings
  • The component should continue working as expected
  • Try to edit/save the General Settings form as well, the success/error notices should show up as expected.

@youknowriad youknowriad added [Feature] Sharing Features and settings for sharing posts across different platforms, including sharing buttons. [Status] In Progress labels Dec 27, 2016
@youknowriad youknowriad self-assigned this Dec 27, 2016
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/sharing-buttons/redux

@youknowriad youknowriad mentioned this pull request Dec 27, 2016
Merged
@youknowriad youknowriad force-pushed the update/sharing-buttons/redux branch from c2dd34c to 98cc4b1 Compare December 28, 2016 08:02
@youknowriad youknowriad requested a review from timmyc December 28, 2016 08:03
@youknowriad youknowriad added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 28, 2016
@youknowriad youknowriad mentioned this pull request Dec 28, 2016
Merged
timmyc
timmyc reviewed Dec 28, 2016
View reviewed changes
Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is testing out well for me on both JP and wpcom sites. I was unable to trigger the IP address notif - I tried malformed IP addresses but it always said "Settings Saved" and indeed upon a hard refresh the settings were still shown

client/my-sites/sharing/controller.js
site: site,
buttons: sharingButtonsList,
postTypes: postTypesList
site: site
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we still passing in site and not getting it from the state tree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nice to have enhancement (that could be done in another PR) is to avoid using the site Prop. This would require refactoring the underlying components.

Nevermind :)

client/my-sites/site-settings/form-general.jsx
let text;
switch ( nextProps.saveRequestError.error ) {
case 'invalid_ip':
text = nextProps.translate( 'One of your IP Addresses was invalid. Please try again.' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By chance do you know how to trigger this error so we can test it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know, I searched and found the error key on the API side but I'm not sure how to reproduce.

This is error was here in the first place, that's why I did not touch it, but maybe worth removing since I don't think the user wants to know about this. We could even drop the "error" and the corresponding selector from the redux state. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grok'ed it - and it looks like invalid_ip error code is part of the domains/DNS logic - I'll try to test out in that area.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to trigger this error in the DNS manager. There is some inline validation that requires xxx.xxx.xxx.xxx format, entering in a bogus IP seems to persist like 255.255.255.0 - anyhow, thinking it is fine to leave as-is

@timmyc
Copy link
Contributor

timmyc commented Dec 30, 2016

After resolving merge conflict this should be good to merge

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 30, 2016
@youknowriad youknowriad force-pushed the update/sharing-buttons/redux branch from 98cc4b1 to dd6053b Compare December 30, 2016 11:01
@youknowriad youknowriad force-pushed the update/sharing-buttons/redux branch from dd6053b to 0ac330a Compare December 30, 2016 11:07
@youknowriad youknowriad merged commit f0cedac into master Dec 30, 2016
@youknowriad youknowriad deleted the update/sharing-buttons/redux branch December 30, 2016 11:17
@youknowriad youknowriad mentioned this pull request Jan 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timmyc timmyc timmyc left review comments

Assignees

@youknowriad youknowriad

Labels
[Feature] Sharing Features and settings for sharing posts across different platforms, including sharing buttons. [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@youknowriad @matticbot @timmyc @lancewillett