Jetpack Section: Backup Screen - Introduce Feature Flag#13709
Jetpack Section: Backup Screen - Introduce Feature Flag#13709
Conversation
The backup menu is being displayed based on the backups feature config. PS: The backup drawable is temporary and will be replaced with a permanent one once the designs are finalized.
This fixes lots of import related warning on this class, thus making it easier to spot a new warning.
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
ashiagr
left a comment
There was a problem hiding this comment.
LGTM @ParaskP7! Also thank you for fixing android R imports 🙏.
Just a note for the future implementation:
(I understand this PR only targets the "old My Site" section.)
We have a ImprovedMySiteFragment which will replace the old MySiteFragment soon. It gets enabled when MySiteImprovementsFeatureConfig is enabled in App Settings.
"Scan" menu is functional only from the improved my site flow right now. We might want to sync in the next step to decide if we want to support new menus on the old My Site as I am not sure when MySiteImprovements is getting released.
|
👋 @ashiagr ! Thank you so much for this review! 🌟
You are totally right, I wasn't aware of that and missed that completely. I am currently updating this PR to add the
|
Oh I thought Scan menu is not connected to the Scan screen if accessed from the |
The build backup list item functionality is based on the backups feature config. PS: The backup drawable is temporary and will be replaced with a permanent one once the designs are finalized.
The build backup list item functionality of the 'SiteItemsBuilder' is triggered based on the backups feature config.
This is because of the fact that the to be added new parameter will not otherwise fit the line.
This is an equivalent to what the previous commit did for the new 'ImprovedMySiteFragment'. As such, this commit is added to keep both screens, the old and the new one, consistent with each other.
|
👋 @ashiagr ! I have uploaded the solution for Many thanks in advance! 🙏 |
ashiagr
left a comment
There was a problem hiding this comment.
Thanks for the changes @ParaskP7! I have verified that backup menu is present on both old and new My Site fragment based on the feature flag & currently irrespective of the site capabilities.
Also thanks for splitting code changes into separate commits and including appropriate information in the commit messages. It made reviewing very easy, good job 🌟!
|
Thanks for approving and merging the PR @ashiagr ! 🌟 I am really glad that you liked the way I structured my commits. I have been using this approach for few years now and been advocating in favor of it. It is really nice to see you found it valuable. In my opinion a reviewer should be aware of every step of the solution, small descriptive commits along with doing commit-by-commit review help navigating to the final outcome and in a sense doing an async pair programming with the author. I feel that this works really good. Also, doing that it is very beneficial for the author itself since they don't get overwhelmed by a major commit, which might include lots of feature additions, along with refactors, maybe tests, etc. With small commits, the author can quickly review their own commit much easier before committing it. PS: Apologies for my mumbling above 😄 I am just really glad you liked it, and thanks again! 🤗 |
Parent #13629
This PR introduces a local feature flag for the
Backupsfeature. When the feature flag is enabled aBackupmenu is shown on theMy Sitetab (currently irrespective of the site plan).To test:
My Sitetab.Backupmenu is hidden.My Site->Toolbar Avatar->App Settings->Test feature configuration.BackupsFeatureConfig, scroll down and click theRESTART THE APPbutton. DO NOT confuse this feature flag with theBackupFeatureConfig(singular). Make sure to enable the right feature flag.My Sitetab and noticeBackupmenu is shown.PR submission checklist:
RELEASE-NOTES.txtif necessary.