Skip to content

Comments

Blogging Prompts: Update the display logic for prompts dashboard card#18816

Merged
wargcm merged 3 commits intotrunkfrom
task/18429-update-card-display-logic
Jun 10, 2022
Merged

Blogging Prompts: Update the display logic for prompts dashboard card#18816
wargcm merged 3 commits intotrunkfrom
task/18429-update-card-display-logic

Conversation

@wargcm
Copy link
Contributor

@wargcm wargcm commented Jun 2, 2022

See: #18429

Description

Updates the prompts dashboard card with the following logic:

  • Show for all sites if the user has prompt reminders enabled
  • If reminders are disabled, show for potential blogging sites
  • Hide skipped prompts

Testing

To test:

  • Enable bloggingPrompts feature flag in FeatureFlag.swift
  • Launch the app and sign in if necessary
  • Select a blogging site
  • Navigate to the 'My Site' tab
  • Navigate to Menu > Site Settings > Blogging Reminders
  • If blogging prompts is enabled, disable it, set a schedule, and tap 'Notify me'/'Update'
  • Navigate back to the dashboard
  • Verify the dashboard card still appears
  • Open the context menu '...'
  • Tap on 'Skip this prompt'
  • Verify the card is hidden
  • Change to a non-blogging site
  • Verify the dashboard card is hidden
  • Navigate to Menu > Site Settings > Blogging Reminders
  • Enable blogging prompts, set a schedule, and tap 'Notify me'/'Update'
  • Navigate back to the dashboard
  • Verify the dashboard card is shown

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 2, 2022

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

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 2, 2022

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

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

@alpavanoglu
Copy link
Contributor

The test case works fine. But I've found two issues:

  1. The button needs to be disabled after first interaction so that duplicate/multiple requests can't be made.
  2. I don't think Notify Me should be enabled if the date/time is not modified from last configuration. Then the user can set the same reminder. It could be useful to highlight there's no change to be applied in that state.
Simulator.Screen.Recording.-.iPhone.13.-.2022-06-09.at.13.33.20.mp4

@wargcm
Copy link
Contributor Author

wargcm commented Jun 9, 2022

@alpavanoglu

The button needs to be disabled after first interaction so that duplicate/multiple requests can't be made.

Good call! Looks like there was something there already but it was only accounting for the network call. I updated it to be as soon as the button is tapped now.

I don't think Notify Me should be enabled if the date/time is not modified from last configuration. Then the user can set the same reminder. It could be useful to highlight there's no change to be applied in that state.

I wasn't really sure about this one. That button also checks for the notification permissions. So if the user disabled notifications on the app and was wondering where their reminders are, tapping the 'Notify me' button again will prompt them to enable notifications for the app. What are your thoughts? I don't feel too strongly one way or the either.

I additionally bumped the minimumScaleFactor on the reminder's cell that you pointed out. It still isn't ideal but looks a little better I think.

@alpavanoglu
Copy link
Contributor

@wargcm if you don't think of it as a direct Win, I think we can skip my other suggestion. As you said it might still have value and the change may not benefit the users considerably. Thanks for taking them into account!

@wargcm wargcm merged commit aad4d2a into trunk Jun 10, 2022
@wargcm wargcm deleted the task/18429-update-card-display-logic branch June 10, 2022 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants