-
Notifications
You must be signed in to change notification settings - Fork 794
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
Publicize: Improve primary form component. #21090
Publicize: Improve primary form component. #21090
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
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.
Just a couple of small suggestions. I'll run the code next to test the functionality.
projects/plugins/jetpack/extensions/plugins/publicize/components/form/index.js
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md
Outdated
Show resolved
Hide resolved
...ts/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md
Outdated
Show resolved
Hide resolved
...ts/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md
Outdated
Show resolved
Hide resolved
...ts/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md
Outdated
Show resolved
Hide resolved
Checked-out and built
✅ Facebook, Twitter, Tumblr, and LinkedIn posts worked as expected! Works as expected! 👍 |
4284dcd
to
4eddd4c
Compare
Caution: This PR has changes that must be merged to WordPress.com |
Thanks, @DustyReagan for your review. Suggestions have been committed 👍 |
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.
projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/index.js
Outdated
Show resolved
Hide resolved
...cts/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/index.js
Outdated
Show resolved
Hide resolved
Pretty sure it was not introduced by this PR. |
It happens in prod right now. Do you'd like to change it? I suggest creating a new issue in that case :-D |
…licize/components/form/index.js Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
…licize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
bcc4987
to
4582e5d
Compare
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.
✅ Facebook, Twitter, Tumblr, and LinkedIn posts worked as expected!
✅ Viewed component using the React dev tool.
✅ Viewed core sub store tree, using the Redux dev tool.
✅ Toggles seem to be working. I verified their state in 'core' sub store tree using Redux dev tool. I also tested manually. With Facebook, Twitter, Tumblr, and LinkedIn connected, I tested the toggles with two new posts. One post I toggled off Facebook and LinkedIn and it correctly published to Twitter and Tumblr. I did the opposite on a second post and it also posted correctly.
LGTM! 👍
projects/plugins/jetpack/extensions/plugins/publicize/components/form/index.js
Outdated
Show resolved
Hide resolved
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 is working for me. I think it has thrown up some other things we might want to change, but as discussed, we should keep this to a refactor PR.
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.
Tests well, looks good!
Great news! One last step: head over to your WordPress.com diff, D67058-code, and commit it. Thank you! |
r232110-wpcom |
* [not verified] publicize: add hooks to deal whit the data flow * [not verified] publicize: re-implement form using function and hooks * [not verified] publicize: remove unwrapped form * [not verified] changelog * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/components/form/index.js Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-select-social-media/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] Update projects/plugins/jetpack/extensions/plugins/publicize/hooks/use-social-media-connections/Readme.md Co-authored-by: Dusty Reagan <dusty@dustyreagan.com> * [not verified] publizice: implement selector inn single hooks * [not verified] publicize: split hooks by message and connections * publicize: fix toggling network issue * publicize: extract hooks vars once Co-authored-by: Dusty Reagan <dusty@dustyreagan.com>
Branched off from #21085
This PR reimplements the form of the Publicize plugin.
Currently, it has two
form
andform-unwrapped
components that make it kind of confusing.With these changes, the form in a single component, in a single file. Also, it adds two React hooks making the whole implementation more readable, at least from my PoV 😅
Fixes #21086
Changes proposed in this Pull Request:
Publicize: Improve primary form.
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Testing the hooks in the Publicize component
You can take a look at the hooks in the
<PublicizeForm />
component using the React dev tool:Testing the data propagation
Similarly, you can take a look at the
core
sub store tree, using the Redux dev tool.The image above shows how the data is stored in the subtree while the user edits the publicize content via the message box component.