Widgets Configuration: Add time interval setting #7873
Conversation
ealeksandrov
changed the title
ealeksandrov
force-pushed
the
issue/7863-widgets-configuration-custom-intent
branch
from
a487ca0 to
a9094a4
Compare
ealeksandrov
force-pushed
the
issue/7863-widgets-configuration-time-interval
branch
from
91d935b to
dbb6279
Compare
You can test the changes from this Pull Request by:
|
Ecarrion
left a comment
Ecarrion
left a comment
There was a problem hiding this comment.
Looks good!
I like this nice and simple improvement 🚀
A couple of extra questions to be on the safe side:
Note: configuration UI still says "Today" in 2 widget title and description, because they are overriden in Localizable.strings.
Would these get updated on the next translation pass or do we need to do something about them?
- How the analytic events would look with these changes? Do we have to update them?
I noticed that big numbers get clipped, I think it is worth looking if we can fit the number somehow!
|
|
||
| /// The maximum number of stats intervals a time range could have. | ||
| var maxNumberOfIntervals: Int { | ||
| switch self { | ||
| case .today: | ||
| return 24 | ||
| case .thisWeek: | ||
| return 7 | ||
| case .thisMonth: | ||
| return 31 | ||
| case .thisYear: | ||
| return 12 | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think all of these properties should be shared in a commonplace with the main stats code?
Since we are going to update the analytics dashboard soon, there is a chance that these go outdated.
There was a problem hiding this comment.
I thought about moving StatsTimeRangeV4 to WooFoundation, but it imports Networking models. Do you think it's ok to allow WooFoundation depend on Networking? Or can we move all related models to WF and make Networking import it?
Original StatsTimeRangeV4 currently lives in Yosemite. I copy-pasted the code to prevent the import of Yosemite+Storage (+ Networking but it's already imported to widgets target).
There was a problem hiding this comment.
WooFoundation depend on Networking
Probs not. Maybe we can move that to an extension helper that is imported in both targets?
There was a problem hiding this comment.
Can it be WooFoundation? I don't like idea of creating another library for a few models.
move all related models to WF and make Networking import it?
- move
StatsTimeRangeV4from Yosemite to WF - move
StatGranularityfrom Networking to WF - import WF everywhere
- remove copy-pasted
StatsTimeRangeand useStatsTimeRangeV4in widgets from WF
There was a problem hiding this comment.
That also work, may involve more work tho!
There was a problem hiding this comment.
Then I'll merge current implementation into the feature branch and we can experiment with dependencies later.
There was a problem hiding this comment.
there is a chance that these go outdated
☝️ What are the chances of this happening? What is the risk? How many times is this duplicated?
Maybe we can also use the Rule of three as a guideline.
Another library is also fine to me, assuming the cost is smaller compared to trying to place it in WooFoundation.
Generated by 🚫 dangerJS |
We should ask release managers when we're ready. One way I see is to remove already translated keys manually to "force" new translation pass. But there may be a better more organized way.
Sure, that's something to think about before finishing. This PR lands on feature branch, not on the trunk yet. I'll see if store picker will be easy to add into this set of changes, so we can consider both parameters in Tracks.
We can use compact presentation in that case? Like |
ealeksandrov
deleted the
issue/7863-widgets-configuration-time-interval
branch
Part of: #7863
Based on: #7872
Description
This PR updates widget data provider to fetch data for user-selected time interval.
It also cleans up code for any hardcoded mention of "today" for the widget.
Note: configuration UI still says "Today" in 2 widget title and description, because they are overriden in
Localizable.strings.Testing
RELEASE-NOTES.txtif necessary.