Skip to content

Extract content logic to config from lock screen widget - #20317

Merged
staskus merged 5 commits into
wordpress-mobile:feature/19411-add-lock-screen-stats-widgetsfrom
tzef:feature/beemotrial/lockscreen-widgetes-config
Mar 22, 2023
Merged

Extract content logic to config from lock screen widget#20317
staskus merged 5 commits into
wordpress-mobile:feature/19411-add-lock-screen-stats-widgetsfrom
tzef:feature/beemotrial/lockscreen-widgetes-config

Conversation

@tzef

@tzef tzef commented Mar 14, 2023

Copy link
Copy Markdown
Contributor

This is the PR for extracting the parameters used in the stats widget into configs

  1. Add new feature flag for lock screen widgets in Jetpack  #20309 (✅ Approved)
  2. Implement lock screen widgets UI #20312 (✅ Approved)
  3. Add dynamic type support #20342 (TBD)
  4. Add localizable string for "Views Today" title #20353 (✅ Approved)
  5. [BugFix] Fix widget of home screen for WordPress disappeared #20371 (✅ Approved)
  6. Extract content logic to config from lock screen widget #20317 (✅ Approved) 👈 you're here!
  7. Create a standalone TimeLineProvider and TimelineEntry for the lock screen  #20368 (✅ Approved)
  8. Implement view for logged-out, no-site, and no-data status #20399 (✅ Approved)
  9. Notify to reload the timelines for lock screen widget #20405 (✅ Approved)
    (Confirm the data display on UI correctly in this phase)
  10. Implement event tracking for the new lock screen widget  #20422 (✅ Approved)
  11. Disable lockScreenWidget feature flag #20427 (In Reviewing)

Description

Extract the content logic from LockScreenStatsWidget to the config LockScreenStatsWidgetConfig.
config and widget is 1-1 relationship, added LockScreenTodayViewsStatWidgetConfig for Today Views widget

This will allow for adding new widgets in the future by simply creating another config to provide to LockScreenStatsWidget, without duplicating another widget.

Note:
To achieve this, we have to inject the config to LockScreenStatsWidget.
but LockScreenStatsWidget conforms to the Widget protocol, which requires the implementation of init().

So I added fatalError and deprecated annotation to init() to avoid a force unwrap for the config in LockScreenStatsWidget.
This will cause the compiler to stop the incorrect construction with an error message:
"'init()' is deprecated: replaced by 'init(config:)'".

    private let config: T

    init(config: T) {
        self.config = config
    }

    @available(*, deprecated, renamed: "init(config:)")
    init() {
        fatalError("Please use init(config: SingleStatWidgetConfig) to provide the config")
    }

Diagram

image

Testing instructions

This change didn't introduce new features

Regression Notes

  1. Potential unintended areas of impact
    Adding a widget to the lock screen and display correctness.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    The testing instruction in previous PRs

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@staskus staskus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I overall agree with this configuration and usage, it makes sense to me:

LockScreenStatsWidget(config: LockScreenTodayViewsStatWidgetConfig())

I'll wait for dependent PRs to be merged so I could review this one cleanly and wouldn't make excessive comments.


@available(iOS 16.0, *)
struct LockScreenStatsWidget<T: LockScreenStatsWidgetConfig>: Widget {
private var config: T!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like since init() implements fatalError then it can be a constant

Suggested change
private var config: T!
private let config: T

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I don't know this!!
Now the syntax looks makes sense 🙌

@tzef

tzef commented Mar 15, 2023

Copy link
Copy Markdown
Contributor Author

Yes, let's focus on the previous PR, after that I will reorg this PR and notify you, thanks

@tzef
tzef marked this pull request as draft March 15, 2023 14:44
@tzef
tzef force-pushed the feature/beemotrial/lockscreen-widgetes-config branch from 58679b4 to ec46b7f Compare March 16, 2023 04:47
@tzef
tzef force-pushed the feature/beemotrial/lockscreen-widgetes-config branch from ec46b7f to 07aa31b Compare March 17, 2023 02:18
@tzef
tzef marked this pull request as ready for review March 17, 2023 03:24
@tzef
tzef marked this pull request as draft March 17, 2023 11:31
@tzef
tzef force-pushed the feature/beemotrial/lockscreen-widgetes-config branch from 07aa31b to f5dd1ea Compare March 20, 2023 03:14
@tzef
tzef marked this pull request as ready for review March 20, 2023 03:16
@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@staskus

staskus commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

@tzef let me know when you want this reviewed, thanks!

@tzef
tzef force-pushed the feature/beemotrial/lockscreen-widgetes-config branch from 5ec75cb to 789d17b Compare March 21, 2023 12:51
@tzef

tzef commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

@staskus I just rebased it, this PR is ready for review now, thanks!

@staskus

staskus commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

@staskus I just rebased it, this PR is ready for review now, thanks!

@tzef Thanks. I'll review it in a bit.

@staskus
staskus self-requested a review March 22, 2023 07:25

@staskus staskus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No surprises here, it looks as I would've imagined. 👍

Good to see this architecture coming alive. It's a nice split:

  • Config for data
  • View for UI
  • ViewProvider for building views for different states

@staskus

staskus commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

@tzef merging

@staskus
staskus merged commit b649301 into wordpress-mobile:feature/19411-add-lock-screen-stats-widgets Mar 22, 2023
staskus added a commit that referenced this pull request Sep 4, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants