Skip to content

Blogging Prompt Compact Card: show a real prompt#18572

Merged
ScoutHarris merged 3 commits intotrunkfrom
feature/18429-compact_card_show_prompt
May 11, 2022
Merged

Blogging Prompt Compact Card: show a real prompt#18572
ScoutHarris merged 3 commits intotrunkfrom
feature/18429-compact_card_show_prompt

Conversation

@ScoutHarris
Copy link
Contributor

@ScoutHarris ScoutHarris commented May 11, 2022

Ref: #18429

The compact prompt card now fetches one prompt and updates the card accordingly.

To test:

  • Enable bloggingPrompts feature flag.
  • Tap on the FAB.
  • Verify the prompt is displayed in the header.
  • HACK: In BloggingPromptsHeaderView:configure, set answered to true.
    func configure(_ prompt: BloggingPrompt?) {
        promptLabel.text = prompt?.text ?? nil

//        let answered = prompt?.answered ?? false
        let answered = true
        
        answerPromptButton.isHidden = answered
        answeredStackView.isHidden = !answered
    }
  • Verify the answered state on the card is toggled correctly.
Not Answered Answered
unanswered answered

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.

@ScoutHarris ScoutHarris added this to the 19.9 milestone May 11, 2022
@ScoutHarris ScoutHarris self-assigned this May 11, 2022
@ScoutHarris ScoutHarris requested a review from wargcm May 11, 2022 19:54
@ScoutHarris ScoutHarris marked this pull request as ready for review May 11, 2022 19:54
@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 11, 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 pr18572-1d169de on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 11, 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 pr18572-1d169de on your iPhone

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

Copy link
Contributor

@wargcm wargcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Only a couple minor things.

}

func configure(_ prompt: BloggingPrompt?) {
promptLabel.text = prompt?.text ?? nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nitpick]: The ?? nil isn't needed here

super.init()

listenForQuickStart()
fetchBloggingPrompt()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also end up being called on the 'Pages' screen since it uses this coordinator. It could probably be mitigated by either checking the presenting view controller or the number of actions. Or alternatively, moving the fetch to the same spot the header is created. What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also end up being called on the 'Pages' screen since it uses this coordinator.

Woops. Forgot that was different.

moving the fetch to the same spot the header is created

That was my original intent. However, since everything was dependent on the fetching, it led to a blackhole of completion blocks. The intent for fetching the prompt in init is to be sure there is a prompt before it is needed.

checking the presenting view controller

This felt a bit more fragile as things could move, the FAB could be added somewhere else, etc.

the number of actions

This felt like the best dynamic option. Door #3 it is!

@ScoutHarris
Copy link
Contributor Author

Thanks @wargcm !

@ScoutHarris ScoutHarris enabled auto-merge May 11, 2022 23:36
@ScoutHarris ScoutHarris merged commit 83eca76 into trunk May 11, 2022
@ScoutHarris ScoutHarris deleted the feature/18429-compact_card_show_prompt branch May 11, 2022 23:54
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