Time Selection Feature #15128

ravishanker · 2021-08-04T08:26:14Z

Implemented time selection for blogging reminders

Fixes #15123

To test:

Go to site settings
Find Blogging reminders setting and click on the item below it
It should launch select days bottom sheet
bottom sheet should now have Notification time item, click on time
It should show time selection dialog
Select a time, AM, PM and time should reflect the selection
De-select all days, time list item should disappear, then select a day, time should appear again

Regression Notes

Potential unintended areas of impact
Reminder scheduling
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
Unit testing

PR submission checklist:

I have completed the Regression Notes.
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.

The text was updated successfully, but these errors were encountered:

Implemented time selection for blogging reminders

peril-wordpress-mobile · 2021-08-04T08:26:19Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-08-04T08:44:26Z

You can test the changes on this Pull Request by downloading the APKs:

ravishanker · 2021-08-05T07:13:32Z

Needs an update to FluxC in a separate PR here

renanferrari

Thanks for the changes! I'm not sure if this is ready for review, but I've taken a quick look and it seems that the time picker is behaving a bit weirdly after I dismiss it by tapping on the outside:

video.7.mp4

Also, I noticed that the time formatting is not following my current locale?

Let me know if this is ready for a proper review, so I can take a deeper look at the code as well.

ravishanker · 2021-08-10T05:09:51Z

Thanks for the changes! I'm not sure if this is ready for review, but I've taken a quick look and it seems that the time picker is behaving a bit weirdly after I dismiss it by tapping on the outside:
video.7.mp4

Also, I noticed that the time formatting is not following my current locale?

Let me know if this is ready for a proper review, so I can take a deeper look at the code as well.

Thanks @renanferrari for a quick look. Good catch on the issue. Looks like if the time picker is cancelled, then it is not dismissing the dialog.

Unfortunately, I couldn't use material MaterialTimePicker even after upgrading material version. MaterialTimePicker is crashing due to bridge theme in use in the app.

I've now fixed the issue with Cancel of TimePicker. You can may be review this after I merge the FluxC PR first, then update this one. Thanks

…tion

renanferrari

Thanks for the changes! After testing, it seems that the dialog behavior is still there if we press the back button. I've taken a look at the code and left a few suggestions below. Let me know what you think.

.../main/java/org/wordpress/android/ui/bloggingreminders/BloggingReminderBottomSheetFragment.kt

...Press/src/main/java/org/wordpress/android/ui/bloggingreminders/BloggingReminderTimePicker.kt

renanferrari · 2021-08-10T19:25:52Z

...Press/src/main/java/org/wordpress/android/ui/bloggingreminders/BloggingReminderTimePicker.kt

+        fun newInstance(hour: Int, minute: Int): BloggingReminderTimePicker {
+            return BloggingReminderTimePicker().apply {
+                arguments = Bundle().apply {
+                    putInt(ARG_HOUR, hour)
+                    putInt(ARG_MINUTE, minute)
+                }
+            }
+        }


It seems that we're not using these arguments anywhere yet. I imagine you're planning to use them to pass the currently selected time to the dialog? If so, I think that's the way to go 👍

Yes, once I merge the FluxC PR

Removed the arguments now, no longer needed as time and hour are available after FluxC update

I believe we should still pass the currently selected time to the dialog. If we don't, the time picker will display a different time from the one that's being displayed on the bottom sheet:

notification-time.mp4

Updated to reflect current time selection on the picker after merging FluxC PR

renanferrari · 2021-08-10T19:25:52Z

WordPress/src/main/java/org/wordpress/android/ui/bloggingreminders/BloggingRemindersItem.kt

@@ -44,5 +52,11 @@ sealed class BloggingRemindersItem(val type: Type) {
        data class DayItem(val text: UiString, val isSelected: Boolean, val onClick: ListItemInteraction)
    }

+    data class TimeItem(
+        val time: UiString,
+        val isInvisible: Boolean = false,


It looks like isInvisible is not being used anywhere?

Need to seek clarification on this one. We had discussion if you remember, whether the time selection should always be displayed or only after a day selection is made. If we decide to default to display always, then this can be removed

Update it now, to be visible only when at least a day is selected

I'm not sure what's the best approach here, but the way we are currently hiding the item looks quite weird IMHO:

notification-time-visibility.mp4

If we want to hide that, we should consider conditionally adding this TimeItem to the list instead of hiding its content. Personally, I'd probably just default to always show it, though.

Either way, we should probably ask for a design review here. You can do that by adding the "Needs design review" label and/or pinging a designer yourself.

Yeah, Chris is AFK, will ask next week when he returns. For now, made TimeItem conditional. Thanks

WordPress/src/main/java/org/wordpress/android/ui/bloggingreminders/BloggingRemindersUiModel.kt

...ress/src/main/java/org/wordpress/android/ui/bloggingreminders/BloggingRemindersViewHolder.kt

refactored time picker handling

Fix ciktlint issue

build.gradle

renanferrari

Left a few more comments above. Let me know if you need a hand with anything!

renanferrari

Sorry, just noticed a few more things!

WordPress/src/main/java/org/wordpress/android/workers/reminder/ReminderScheduler.kt

WordPress/src/main/java/org/wordpress/android/ui/bloggingreminders/DaySelectionBuilder.kt

Refactored and updated after merging FluxC PR

Track time selection feature by adding a selected_time property to the blogging_reminders_scheduled event

renanferrari

Great work!

LGTM

Feel free to merge this after resolving the merge conflicts. I think we can address any design issues separately as well, but it's up to you.

…tion

fix lint checks

…vents-for-blogging-reminders-v2 Add tracking for time selection

with FluxCVersion

osullivanchris · 2021-08-18T08:22:59Z

Hey @renanferrari @ravishanker I had a think about the time display when the user has not selected any days yet.

What if we display it but disabled until days are selected? Then we follow the same rational that its not interactive before days are selected, but, the UI doesn't jump when it becomes active. WDYT?

The only thing I wasn't sure is if we should display the default time, or no time, before a day is selected.

I also thought Ravi had a nice idea about using a button style for the time to make it more clear that its interactive. Here's a quick mock. I think it looks pretty nice but I'm not sure if its ok to put a text button in a list item like this?

osullivanchris · 2021-08-19T08:36:48Z

As discussed elsewhere, we have good reason for 'hiding' rather than disabling the time initially. So we can ignore my above suggestion for now!

Time Selection Feature

2a90f3d

Implemented time selection for blogging reminders

ravishanker added Blogging Reminders [Type] Task labels Aug 4, 2021

ravishanker added 4 commits Aug 5, 2021

Fix lint errors

a658a81

Fix failing tests

66daf10

fix ktlint issues

07adecd

fix ciktlint issues

5dc045e

ravishanker requested a review from renanferrari Aug 5, 2021

Update BloggingRemindersModelMapper.kt

a9e9b27

renanferrari reviewed Aug 9, 2021

View changes

ravishanker added 2 commits Aug 10, 2021

Merge branch 'develop' into feature/15123-blogging-remindertime-selec…

ba219d3

…tion

fix time picker cancellation

62ad150

renanferrari reviewed Aug 10, 2021

View changes

ravishanker added 2 commits Aug 11, 2021

Update for review

db39b1d

refactored time picker handling

Update BloggingRemindersViewModel.kt

9570d8a

Fix ciktlint issue

ravishanker marked this pull request as ready for review Aug 11, 2021

ravishanker reviewed Aug 11, 2021

View changes

build.gradle Show resolved Hide resolved

Loading

renanferrari reviewed Aug 11, 2021

View changes

WordPress/src/main/java/org/wordpress/android/workers/reminder/ReminderScheduler.kt Outdated Show resolved Hide resolved

Loading

WordPress/src/main/java/org/wordpress/android/ui/bloggingreminders/DaySelectionBuilder.kt Outdated Show resolved Hide resolved

Loading

ravishanker added this to the 18.1 milestone Aug 12, 2021

ravishanker added 4 commits Aug 12, 2021

Refactor and Update after FluxC

cd42b9c

Refactored and updated after merging FluxC PR

fix ciktlint issue

ed783ac

Add tracking for time selection

46baf29

Track time selection feature by adding a selected_time property to the blogging_reminders_scheduled event

fix test

3e75ace

renanferrari approved these changes Aug 13, 2021

View changes

Merge branch 'develop' into feature/15123-blogging-remindertime-selec…

988e321

…tion

ravishanker and others added 4 commits Aug 16, 2021

Update tracking time to 24 h format

2fda6bb

Update BloggingRemindersViewModel.kt

2a47388

fix lint checks

Merge pull request #15178 from wordpress-mobile/feature/15177-track-e…

256cd4d

…vents-for-blogging-reminders-v2 Add tracking for time selection

Update build.gradle

15175a6

with FluxCVersion

ravishanker merged commit fd04a08 into develop Aug 16, 2021
13 checks passed

Loading

ravishanker deleted the feature/15123-blogging-remindertime-selection branch Aug 16, 2021

wordpress-mobile / WordPress-Android Public

Time Selection Feature #15128

Time Selection Feature #15128

ravishanker commented Aug 4, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 4, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 4, 2021 •

edited

Loading

ravishanker commented Aug 5, 2021 •

edited

Loading

renanferrari left a comment

ravishanker commented Aug 10, 2021 •

edited

Loading

renanferrari left a comment

renanferrari Aug 10, 2021

ravishanker Aug 11, 2021

ravishanker Aug 11, 2021

renanferrari Aug 11, 2021

ravishanker Aug 12, 2021 •

edited

Loading

renanferrari Aug 10, 2021

ravishanker Aug 11, 2021

ravishanker Aug 11, 2021

renanferrari Aug 11, 2021

ravishanker Aug 12, 2021

renanferrari left a comment

renanferrari left a comment

renanferrari left a comment •

edited

Loading

osullivanchris commented Aug 18, 2021

osullivanchris commented Aug 19, 2021

wordpress-mobile / WordPress-Android Public

Time Selection Feature #15128

Time Selection Feature #15128

Conversation

ravishanker commented Aug 4, 2021 • edited Loading

Regression Notes

peril-wordpress-mobile bot commented Aug 4, 2021 • edited Loading

peril-wordpress-mobile bot commented Aug 4, 2021 • edited Loading

ravishanker commented Aug 5, 2021 • edited Loading

renanferrari left a comment

ravishanker commented Aug 10, 2021 • edited Loading

renanferrari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravishanker Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renanferrari left a comment

renanferrari left a comment

renanferrari left a comment • edited Loading

osullivanchris commented Aug 18, 2021

osullivanchris commented Aug 19, 2021

ravishanker commented Aug 4, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 4, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 4, 2021 •

edited

Loading

ravishanker commented Aug 5, 2021 •

edited

Loading

ravishanker commented Aug 10, 2021 •

edited

Loading

ravishanker Aug 12, 2021 •

edited

Loading

renanferrari left a comment •

edited

Loading