Skip to content
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

Move Notification Settings to Notifications tab #12287

Merged
merged 6 commits into from Aug 21, 2019

Conversation

@frosty
Copy link
Contributor

commented Aug 8, 2019

This PR implements an item from the IA project – moving Notification Settings from the Me tab to the Notifications tab. Android PR is here: wordpress-mobile/WordPress-Android#10335

notifications

To test:

  • Build and run
  • Ensure that Notification Settings is no longer present in the Me tab
  • Ensure that the cog icon is present in the Notifications screen when: you are signed into a wpcom site or a Jetpack site, but not when you are just signed into a non-Jetpack self-hosted site
  • Tap the icon and ensure Notification Settings are displayed and usable
  • Check universal links still work – with the simulator open, in Terminal run: xcrun simctl openurl booted https://wordpress.com/me/notifications. This should launch Notification Settings.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
frosty added 2 commits Aug 2, 2019

@frosty frosty added this to the 13.1 milestone Aug 8, 2019

@frosty frosty requested a review from danielebogo Aug 8, 2019

@iamthomasbishop

This comment has been minimized.

Copy link

commented Aug 8, 2019

Design-wise this looks mostly solid, and this will make folks using the app happy 😄 One question regarding:

Ensure that Notification Settings is no longer present in the Me tab

My feelings on this aren't strong, but I think we might want to consider leaving the Notifications Settings cell on the Me page temporarily, so we can measure its usefulness against the new entry point. I understand keeping the Notification Settings in that table would mean two entry points, but I think it might be a good thing to measure. I'd love to hear others' opinions on the topic, though.

@danielebogo
Copy link
Contributor

left a comment

Hey @frosty everything looks great! I tested it and it worked perfectly! I left only a nitpick but a part from that :shipit:

One thing I noticed, but it's not related to this PR, if you receive the push notification while there is already a vc presented you get this warning: Warning: Attempt to present <UINavigationController: 0x7fd05d812e00> on <WordPress.NotificationsViewController: 0x7fd05d073a00> whose view is not in the window hierarchy!

@@ -1190,6 +1220,10 @@ private extension NotificationsViewController {
return AccountHelper.isDotcomAvailable() == false
}

var shouldDisplaySettingsButton: Bool {
return AccountHelper.isDotcomAvailable() == true

This comment has been minimized.

Copy link
@danielebogo

danielebogo Aug 8, 2019

Contributor

Can this one returns directly AccountHelper.isDotcomAvailable()?

This comment has been minimized.

Copy link
@danielebogo

danielebogo Aug 8, 2019

Contributor

I know it's not related to this PR but I think also some line above you can do something like:

var shouldDisplayJetpackPrompt: Bool {
        return !AccountHelper.isDotcomAvailable()
    }

This comment has been minimized.

Copy link
@frosty

frosty Aug 9, 2019

Author Contributor

Right, for the second item I think some folks do == false instead of ! because it can be more obvious to read at a glance (it could be easy to not see the ! at the start).

@danielebogo
Copy link
Contributor

left a comment

:shipit:

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 13, 2019

My feelings on this aren't strong, but I think we might want to consider leaving the Notifications Settings cell on the Me page temporarily, so we can measure its usefulness against the new entry point. I understand keeping the Notification Settings in that table would mean two entry points, but I think it might be a good thing to measure. I'd love to hear others' opinions on the topic, though.

Hey @iamthomasbishop back from AFK and catching up on this one. I agree it would be a good thing to measure. But of all the things we could measure I'm not sure this is one to prioritise. My reasons:

  • Its unlikely we will change our approach
  • User testing indicated this was a clearer entry point
  • Two entry points while small is a complexity we have to remember, support, and clean up later
  • Its not perfect but we can by proxy understand the effect by seeing how many more people use the feature after the change (I know this isn't very good science but should be enough in this case)

Hope you're ok with that!

@iamthomasbishop

This comment has been minimized.

Copy link

commented Aug 13, 2019

@osullivanchris All good points, my suggestion was mainly out of curiosity, to run as a short test in production – in other words, keep both to gather some data for a short period of time (~1 week?) to see the effects. This isn't a strong rationale, however, so it's fine by me if we decide to skip this altogether.

@loremattei loremattei modified the milestones: 13.1 ❄️, 13.2 Aug 14, 2019

@loremattei

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

I'm moving this to 13.2 since 13.1 has been cut. If you want this to make it to 13.1, please move it back, target the release/13.1 branch and ping me to build a new beta.

@frosty

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@osullivanchris I've added the footer to the Me screen with the suggested text. I made one slight tweak to the text, adding 'now' in there to indicate that we just moved it. Look ok?

Simulator Screen Shot - iPhone X - 2019-08-15 at 15 31 25

@iamthomasbishop

This comment has been minimized.

Copy link

commented Aug 15, 2019

@frosty I like this, but what do you think about shortening the label to something along the lines of Notification Settings are now in the Notifications tab or Notification Settings can be found in the Notifications tab?

@frosty frosty merged commit 0a8cd67 into develop Aug 21, 2019

6 checks passed

Hound No violations found. Woof!
Peril All green. Nice work.
Details
ci/circleci: Build UI Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad 6th generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone Xs) Your tests passed on CircleCI!
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

@frosty frosty deleted the feature/notifications-settings-move branch Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.