-
Notifications
You must be signed in to change notification settings - Fork 121
Analytics Hub: Integrate Custom Range Selector UI #8342
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
You can test the changes from this Pull Request by:
|
rachelmcr
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.
This is a nice solution and works great for the most part!
It's a little awkward because of how the checkmark selection appears (or rather, doesn't appear) when a custom date range is selected. It would be nice to improve that as much as possible.
| private func internalSelectionBinding() -> Binding<AnalyticsHubTimeRangeSelection.SelectionType> { | ||
| .init( | ||
| get: { | ||
| selectionType |
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.
This internal selection binding is really clever!
One small issue with this getter: Because the list is populated with AnalyticsHubTimeRangeSelection.SelectionType.allCases and the Custom row is defined as .custom(start: nil, end: nil), the Custom row never shows as selected in the list. If you select a custom date range, and then open the date range selector again, nothing is selected:
One solution could be to replace the getter with something like this:
| selectionType | |
| switch selectionType { | |
| // If a `custom` case is set return one with nil values so the Custom row is selected | |
| case .custom: | |
| return .custom(start: nil, end: nil) | |
| default: | |
| return selectionType | |
| } |
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 think this will be solved more elegantly once we decouple the Business options from the UI options.
I'll make sure handle that on #8340
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.
In the mean time, I've adopted to suggestion as it does not hunt to have it right now!
6658e68 to
ff2b852
Compare
|
Thanks for the review @rachelmcr |
rachelmcr
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.
That sounds like a great plan! 👍

Closes: #8200
Why
This PR connects the custom range selector and passes the selected ranges down to the view model.
How
Update Range Card View to present the custom range selector when the custom range type is selected.
Makes use of an internal binding in the range card view to track when the custom range type is pressed in order to not pass that immediately to the view model but to wait for the selected range.
Demo
Demo.mov
Testing
Note: There are two couple of known issues being tracked here
RELEASE-NOTES.txtif necessary.