-
Notifications
You must be signed in to change notification settings - Fork 798
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
JITMs: add component to be used in the Jetpack Dashboard. #10759
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: January 10, 2019. Generated by 🚫 dangerJS |
|
We can split this work in two PRs at least. This is how we've done a few things...
It would be cool to at least provide one or two unit tests for the redux piece of code. they're straightforward if you take another one as example. This usually speeds up work because we can easily merge the redux work if the tests pass. |
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.
I think I commented all of the previous things without using a Review.
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.
LGTM!
496f8dc
to
4d18803
Compare
9035151
to
575efb0
Compare
…he JITM component of the Jetpack Dashboard (Spin-off of #10759). (#10818) This PR is an extract of #10759 in order to simplify that PR and scope it to UI changes. Author: @jeherve #### Changes proposed in this Pull Request: * Introduces `_inc/client/state/jitm` and files for reducer, actions and tests. * Introduces method `fetchJitm` in the REST API client. #### Testing instructions: * Checkout this branch * Run `yarn test-client _inc/client/state/jitm/test/*` * Confirm tests pass #### Proposed changelog entry for your changes: None needed
This is only needed if one wants to refer to a prop after destructuring instead of referring to it as this.props.isModuleActivated, so to avoid collision of name scopes
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.
LGTM!
Closing this in favor of #10889 |
Previous changes will be reverted in #10890 |
#### Changes proposed in this Pull Request: Revert "JITMs: add redux structure and REST API client method to be u…sed by the JITM component of the Jetpack Dashboard (Spin-off of #10759). (#10818)" This reverts commit dabff02. We will take a different approach, outlined in #10889 #### Testing instructions: * None #### Proposed changelog entry for your changes: * None
Changes proposed in this Pull Request:
JITMs: add component to be used in the Jetpack Dashboard.
The first part of this PR (the necessary new method in the rest api client and the new redux structure) was merged in #10818. This PR brings the second part: a new Jitm component and a new QueryJitm component, as well as a call to the new Jitm component in the main dashboard.
Testing instructions:
JETPACK__SANDBOX_DOMAIN
constant (reference).sandboxme.wordpress.com
as the sandbox you want to use.Known issues / To do:
toplevel_page_jetpack
should continue to work as a string one can use to target all pages of the Jetpack dashboard at once.Lists are not displayed properly right now.Props warnings forisActivatingModule
andisModuleActivated
When dismissing a message, and then switching to a different tab, the dismissed message is visible for a few seconds until the new message (or no message at all) is loaded.When activating a module via a JITM, the JITM remains in place once the module has been activated. This is related to JITM: Hide JITMs whose CTAs were clicked recently #9462I opted to remove the button once the module has been activated. This way one can still click on the CTA to learn more if they are interested. If they leave the page or refresh, the JITM will disappear for good.The look of the messages is a bit different from the ones you'll find in other places in wp-admin. The CTA button uses Calypso's button component, and as such the text inside the button is in uppercase. That is not the case with the wp-admin implementation of the button.Proposed changelog entry for your changes: