-
Notifications
You must be signed in to change notification settings - Fork 121
[Analytics Hub] Add fake order stats for configuring Analytics cards #8191
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
Conversation
Generated by 🚫 dangerJS |
Ecarrion
left a comment
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.
Code looks good but I fail to see the benefit of this fake data, is it to test the helpers? 🤔
|
|
||
| /// Order stats for the current selected time period | ||
| /// | ||
| @Published private var currentOrderStats: OrderStatsV4 = { |
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.
What's the reason for these to be published properties? 🤔
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.
I thought if we make them published properties, we can listen for changes to the data (i.e. when a different time period is selected and new data is synced) to update what is displayed on the cards. WDYT?
|
@Ecarrion I thought it would be useful so we can split the work of integrating the Yosemite layer (retrieving real Is there a better way to split up that work so we can do it in parallel? I'm open to anything; this just seemed like a simple solution. |
|
It just occurred to me that we don't need all the fake data to get started with this if we just set the order data properties to /// Order stats for the current selected time period
///
@Published private var currentOrderStats: OrderStatsV4? = nil
/// Order stats for the previous time period (for comparison)
///
@Published private var previousOrderStats: OrderStatsV4? = nilThis is simpler and should still be appropriate as a starting point for splitting up the work (we won't have any order data until we perform the remote sync, so the view model should be initialized with |
|
Hmmm, I don't know 😓 I was thinking that the "how we store the data" was going to depend on how we fetch it. For example, on my mind, we would have done something like rangeTrigger()
.fetchStatsData()
.bindToCardsVMs()So if we go with that route, we don't need to store the stats data response, does that make sense? In any case, we can always delete this if it doesn't serve our purpose, so feel free to merge it as it wouldn't cause much harm, I hope 😛 |
|
Ah, gotcha. Because the stats store doesn't return the stats response directly (only upserts it into storage) I figured we'd store the data in the view model and do the fetching separately from the binding to card VMs: rangeTrigger()
.fetchStatsData()
.updateDataPropsFromStorage()
observeDataProps()
.maptoCardVMs()But as you said we can delete this later if it doesn't serve our purpose, and any work that we've done in parallel could be stitched together if we decide not to store the data like this. I'll make the change I mentioned earlier and then merge this. |
You can test the changes from this Pull Request by:
|
Closes: #8190
Description
This PR adds a set of fake
OrderStatsv4data we can use to configure the Revenue and Order Analytics cards until real data is integrated into the Analytics Hub (in #8189).Submitter Checklist
Update release notes:
RELEASE-NOTES.txtif necessary.