-
Notifications
You must be signed in to change notification settings - Fork 800
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
Updates Backup launch price UX #14203
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: January 14, 2020. |
Fixed all issues flagged for me, let me know if there's anything missing or more optimal ways to structure things. Thanks! |
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.
Great work, @ChaosExAnima!
I reviewed the code and left some suggestions. I don't there are any blockers, but I think they are worth considering in follow-up pushes.
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 well, nice work @ChaosExAnima!
It's a big PR though, I would have loved it if we could some of the following in separate PRs:
- Billing timeframe handling and logic
- Componentization - moving/building functionality to subcomponents
I've also left some comments and questions, let me know what you think.
8b9ab85
to
b5fbcb7
Compare
f36b1d0
to
b35dd10
Compare
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the updates, @ChaosExAnima! I left just a minor comment. Other than that (and Marin's feedback), I think it's good to go 👍 |
Addressed all the feedback. This is ready for another round of reviews - @delawski mind taking another 👀 ? |
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.
Everything looks good to me now! Let's 🚢it!
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 looks good now. @delawski can you take a look and ✅ it too before 🚢 ?
It's better now, @tyxla 👍Let's have it like this and ship! |
* Changelog: 8.1 additions * Changelog: add #13858 * Changelog: add #13963 * Changelog: add #14174 * Changelog: add #14178 * Changelog: add #14175 * Changelog: add #14192 * Changelog: add #14196 * Changelog: add #14182 * Changelog: add #14218 * Changelog: add #14214 * Changelog: add #13757 * Changelog: add #14190 * Changelog: add #14131 * Changelog: add #14101 * Changelog: add #14203 * Changelog: add #14211 * Changelog: add #14224 * Changelog: add #14230 * Changelog: add #14241 * Changelog: add #14249 * Changelog: add #14264 * Changelog: add #14263 * Changelog: add #14256 * Changelog: add #10189 * Changelog: add #14240 * Changelog: add #14239 Also added some new entries to the testing file. Co-authored-by: Igor Zinovyev <[email protected]>
* Changelog: 8.1 additions * Changelog: add #13858 * Changelog: add #13963 * Changelog: add #14174 * Changelog: add #14178 * Changelog: add #14175 * Changelog: add #14192 * Changelog: add #14196 * Changelog: add #14182 * Changelog: add #14218 * Changelog: add #14214 * Changelog: add #13757 * Changelog: add #14190 * Changelog: add #14131 * Changelog: add #14101 * Changelog: add #14203 * Changelog: add #14211 * Changelog: add #14224 * Changelog: add #14230 * Changelog: add #14241 * Changelog: add #14249 * Changelog: add #14264 * Changelog: add #14263 * Changelog: add #14256 * Changelog: add #10189 * Changelog: add #14240 * Changelog: add #14239 Also added some new entries to the testing file. Co-authored-by: Igor Zinovyev <[email protected]>
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
Note: Functionality for switching between yearly and monthly states is not in scope of this PR.
Before (desktop): https://d.pr/i/R5KeBx
Before (mobile): https://d.pr/i/97e3qz
After (desktop): https://d.pr/i/qnMG5O
After (mobile): https://d.pr/i/amep9G
After, monthly: https://d.pr/i/psGySU
Proposed changelog entry for your changes: