Skip to content

Conversation

@Ecarrion
Copy link
Contributor

@Ecarrion Ecarrion commented Dec 7, 2022

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

  • Open the analytics hub
  • Select a custom time range
  • See that the custom range is loaded.

Note: There are two couple of known issues being tracked here


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@Ecarrion Ecarrion added the category: tracks Related to analytics, including Tracks Events. label Dec 7, 2022
@Ecarrion Ecarrion added this to the 11.6 milestone Dec 7, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 7, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8342-ae63864 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@rachelmcr rachelmcr self-assigned this Dec 8, 2022
Copy link
Contributor

@rachelmcr rachelmcr left a 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
Copy link
Contributor

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:

Suggested change
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
}

Copy link
Contributor Author

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

Copy link
Contributor Author

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!

@ThomazFB ThomazFB linked an issue Dec 8, 2022 that may be closed by this pull request
@oguzkocer oguzkocer modified the milestones: 11.6, 11.7 Dec 9, 2022
@Ecarrion Ecarrion force-pushed the issue/8200-present-UI branch from 6658e68 to ff2b852 Compare December 12, 2022 20:36
@Ecarrion Ecarrion requested a review from rachelmcr December 12, 2022 20:40
@Ecarrion
Copy link
Contributor Author

Thanks for the review @rachelmcr
I've adopted your suggestion but will be trying to make it a bit more clear on #8340

Copy link
Contributor

@rachelmcr rachelmcr left a 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! 👍

Base automatically changed from issue/8200-ranges-support to trunk December 13, 2022 12:08
@Ecarrion Ecarrion merged commit d744aaa into trunk Dec 13, 2022
@Ecarrion Ecarrion deleted the issue/8200-present-UI branch December 13, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: tracks Related to analytics, including Tracks Events.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Analytics Hub] Add custom date range selection support

5 participants