-
Notifications
You must be signed in to change notification settings - Fork 809
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
Backup: Add backup storage UI #28085
Conversation
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run |
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:
Backup plugin:
|
b1f220a
to
cf7fc16
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.
First off, thank you so much for all the work that went into this, @Initsogar! The detailed instructions are wonderful as well.
Everything looks great to me in the testing that I did. Noticed some minor things which I'll list below, but none are blocking; more like nice to haves if we want to create follow-up PRs.
Zero character shows up — bug?
While the first backup is being created a zero (0
) appears in the section created for the storage UI; see below:
Grid positioning
The new storage UI seems to take 6 columns of the existing grid, instead of the last 5 as intended. This one is on me for not having documented this more in detail...sorry about that.
See the following image where the top is the design and below what's in the PR right now, with a red rectangle of where the storage UI should be placed.
This might seem confusing or counter-intuitive, but by adding a full empty column in between the left and right sections, we're creating more opportunities to draw the eye to the storage UI.
Heading in warning state
Saw that you added variable headings for different states — very nice touch. The warning state is missing a custom heading though. Wonder if we could add this one:
Cloud storage is almost full
@keoshi Great! Thanks for the feedback.
|
projects/packages/backup/src/js/components/backup-storage-space/index.jsx
Outdated
Show resolved
Hide resolved
...ckup/src/js/components/backup-storage-space/storage-usage-details/use-storage-usage-text.tsx
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.
Looks good to me!
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.
One minor suggestion inline. Otherwise looks ok to me from a monorepo perspective.
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Changes proposed in this Pull Request:
What's next?
The following features will be address in a new PR:
Screenshots
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No
Unit testing:
You can run unit testing using the following command:
jetpack test packages/backup js
jetpack test packages/backup php
Testing instructions:
add/backup-storage-ui
with a backup plan like Jetpack Backup (1 GB or 10 GB).To try different scenarios for the storage meter, you could try to upload big files to your site or dispatch the events using Redux DevTools (chrome extension). After installing it, follow these steps:
jetpack-backup-plugin
>_
option to show the dispatcher.