Implement lock screen widgets UI - #20312
Conversation
a076a68 to
e0a639c
Compare
Generated by 🚫 dangerJS |
staskus
left a comment
staskus
left a comment
There was a problem hiding this comment.
👍 Thank you for the PR! I'm leaving a couple of comments to discuss.
🔨 Since we're reviewing the code as well as testing the solution, we're used to adding Testing instructions to Pull Requests. The Acceptance Criteria is also great but it doesn't always explain "how" to make a change or reach a certain change, to the instructions can be helpful! Also, the recordings could be provided showing how it's achieved.
✅ When the user has not logged in or without the default website, the widget on the selection page should display placeholder data for the user's reference.
- The selection page shows me default site correctly, however after selecting the widget, the widget appears with "Build later" until I select a site from the configuration.
✅ When the user adds a widget and had more than one website, should be able to switch to another website in the widget configuration and display the corresponding stats data.
- I agree that it's not intuitive how-to configuration is reached. Although it's out of scope.
✅ When the user clicks the widget, should redirect to today stats page for the widget-selected website
Add view provider for handling building view for site selected status Add ViewModelMapper for converting viewModel from home widget data
…orrectly Add mapper and viewModel to WordPress target because app extension is not able to run unitTest
9700763 to
4b9566f
Compare
|
Thank you for the solid suggestions, and sorry for any mistakes due to not being careful enough. I have updated the format of the testing instructions to follow the convention. Regarding the issue of data not coming from the default website, it works on my simulator, but I believe it exists because I have not yet handled the data refresh part. Therefore, I suggest we limit the testing to the UI perspective. I will put all related test cases in the PR about handling data refresh. Additionally, I suggest we also move the language concern to the end of the process. It would be beneficial to have input from the designer so I can raise a design revision post to see if anything could be improved based on the first version. For other issues, I have already fixed and reorganized the commits. Please let me know if I missed anything. Thank you. |
@tzef I see that many questions come from the fact that I'm not sure if it's intended to be implemented now or in a later PR. It would be helpful to track a list of the done, ongoing, and planned tasks for these rectangular stats widgets so I would understand the thinking. As an example, you could create a PR for a parent branch where you would list all the tasks. This way you wouldn't need to list these task dependencies in the every PR you make. How does that sound? |
i12n is an important topic so we should definitely think about it when building UIs. Clarifying with the designers is a good idea but we shouldn't be afraid of making decisions ourselves if we see a problem! 👍 |
That's a great idea. Especially we did step-by-step implementation for the feature, I can't create PR for the parent branch feature/19411-add-lock-screen-stats-widgets due to I'm not the owner, so I will update the roadmap to every PRs and highlight the progress.
Understand, then for this case, I think we can create a new key for "views today" to make sure this style works well, for other fields with different date ranges can postpone after the new widget is decided. And I'm not familiar with the new translation task flow here, I guess we also have a remote service (such as webtranslateit, lokalise) and automatically sync the key and value for each release process? I will try to find the related document, if you can provide the link that'd be helpful, thanks 🙏 In the meanwhile, I will raise this concern to Jetpack to ask for the designer's input, does it good for you? |
staskus
left a comment
staskus
left a comment
There was a problem hiding this comment.
Thanks for the changes and for adding TODOs for the tasks unrelated to UI! 👍
I would still want to see if we could improve the accessibility of this widget in the scope of the UI. Left the comment next to the code.
Thanks!
These are the localization guidelines in our repository docs: https://github.com/wordpress-mobile/WordPress-iOS/blob/trunk/docs/localization.md. You don't need to worry about the remote localization services, everything is done during the release process.
Yes, it works. |
staskus
left a comment
staskus
left a comment
There was a problem hiding this comment.
Okay, thanks for all the answers and including a list of planned tasks on the top of PR.
We can move on to the other PRs and other work.
8bb8533
into
wordpress-mobile:feature/19411-add-lock-screen-stats-widgets
|
@tzef I merged the PR. |
* Add new feature flag for lock screen widgets in Jetpack (#20309) * Add new feature flag for widget in lock screen * Add new widget kind to widget configuration * Add empty lock screen widget * Add JETPACK_STATS_WIDGETS compilation condition To prevent lock screen widgets from displaying on WordPress target * Move LockScreenStatsWidgetsView to views folder group * Revert compilation condition, change to isJetpack of AppCondiguration * Implement lock screen widgets UI (#20312) * Extract statsUrl from private to extension for reusable with lock screen widget * Implement single stat view for today views widget Add view provider for handling building view for site selected status Add ViewModelMapper for converting viewModel from home widget data * Update LockScreenStatsWidgetsView body to get the view from view provider * Add mapper test to verify the viewModel converting from widget data correctly Add mapper and viewModel to WordPress target because app extension is not able to run unitTest * Fix getStatsURL typo * Add localizable string for "Views Today" title (#20353) * Add views in today localizable string for footer label * Remove dateRange and footer, only title is enough for existing feature * Fix home screen widget for WordPress target disappeared. (#20371) The root cause is the WidgetBundleBuilder body doesn't allow control flow statements, move it to supportedFamilies with feature flag to keep all control logic in the same place * Extract content logic to config from lock screen widget (#20317) * Update title of view provider from hardcoded to pass from config * Add lock screen stats widget config and implementation to include required information * Pass config to LockScreenStatsWidget Replaced the hardcoded data with config properties * Add associated type view for other status and TODO comments * Extract app configuration and feature flag check into config * Create a standalone TimeLineProvider and TimelineEntry for the lock screen (#20368) * Add widget data and timeline entry for lock screen to separate concerns * Add lock screen time line provider for lock screen stats widget entry Update stats widgets service result type to consist of home widget and lock screen widget for reusable * Update to use LockScreenSiteListProvider in LockScreenStatsWidget Update associated type widget data to conform `LockScreenStatsWidgetData` more Update model mapper and view provider to based on lock screen widget data Remove unnecessary `disabled` status from `LockScreenStatsWidgetsView` Remove unnecessary `widgetKind` parameter from `LockScreenStatsWidget` Remove unused statsURL converter from the view model mapper Add home widget data conform lock screen widget data extension to WordPress target for unitTest * Extract widget load data to loader for sharing with lock screen time line provider * Update widget reader to extract the cache read from getSnapshot * Extract noData, noSite, loggedOut, disabled status checking logic to data reader with status callback * Add unitTest for widget data reader Extract cache reading to `HomeWidgetDataFileReader` for mocking the testing object Extract app configuration is jetpack to parameter for mocking the config * Remove multiple space lines * Update multiple callback to Result and enum Error * Implement view for logged-out, no-site, and no-data status (#20399) * Add unconfigured view for lockscreen widget * Add unconfigured view model to view provider and model mapper * Add unconfigured view model unit test * Add background to unconfigured view align with success status widget appearance * Remove todo comments * Add reload timelines for lock screen widget (#20405) * Implement event tracking for the new lock screen widget (#20422) * Add event key to configuration and pass to trackers in lockscreen widget Expand tracks for lockscreen stats entry Add rectangular widgets count and widgets update of the new widget * Add lockscreen_widget source to deeplink tracked Update statsUrl to widgetUrl for separating the url from homescreen widgets * Update unitTests * Remove separate widget url, move widget source from widget data extension Add widget source type, assign the type in widgetURL in the view * Add widget url source test to verify the converting with DeepLinkSource * Disable the feature flag (#20427) --------- Co-authored-by: Beemo Lee <tzef8220@gmail.com>
2 participants
This is the second PR for adding the stats widget to the lock screen
minimumScaleFactor(DONE)PreviewProviderforLockScreenSingleStatView(DONE)(Confirm the data display on UI correctly in this phase)
Description
LockScreenStatsWidgetsViewfrom the content logic. Other widgets in the future could reuse the same view provider for displaying different data, such ascommentsandlikesin the same layout.ViewModelandViewModelMapperand added to the WordPress target for unit testing because the app extension is not able to run unit tests for now.statsUrlfor home widget data from private to the extension to share with lock screen widgets, also add the extension to WordPress target for testable.Images & Videos
Testing instructions
For the issue about widget not reflecting the default website data, it might relate to cache or refresh notification in the data store, which I'm not pretty sure about for now, but I think can defer the bug to the PR about handling the refresh mechanism, let this PR focus on UI rendering.
When the user has not logged in or without the default website:
the widget on the selection page should display placeholder data for the user's reference.
When the user logs in and has a default website:
the widget on the selection page should display the stats data from the default website.
When the user adds a widget and had more than one website:
should be able to switch to another website in the widget configuration and display the corresponding stats data.
When the user clicks the widget:
should redirect to today stats page for the widget selected website
Regression Notes
Potential unintended areas of impact
Adding lock screen widget flow
What I did to test those areas of impact (or what existing automated tests I relied on)
The testing instructions in PR Add new feature flag for lock screen widgets in Jetpack #20309
What automated tests I added (or what prevented me from doing so)
WidgetsViewModelMapperTeststo test that the view model was correctly mapped from home widget data.PR submission checklist:
RELEASE-NOTES.txtif necessary.