Skip to content
This repository has been archived by the owner on Feb 13, 2023. It is now read-only.

feat: Remove an answer #144

Merged
merged 24 commits into from
May 14, 2020
Merged

feat: Remove an answer #144

merged 24 commits into from
May 14, 2020

Conversation

richarddubay
Copy link
Member

DESCRIPTION

What does this PR do, or why is it needed?

This PR adds the ability to remove an answer to a prayer.

remove-answer

How do I test this PR?

  • Open up My Prayers
  • Find a prayer that doesn't have an answer
  • Tap on the ellipsis icon and choose "Mark as Answered"
  • On the answer prayer screen you should NOT see an option for "Remove answer"
  • Enter an answer and tap "Save answer"
  • The answer prayer screen should close and take you back to My Prayers. The answer to the prayer should show on the prayer.
  • Tap the ellipsis icon again and choose "Edit Answer".
  • On the answer prayer screen there should now be an option for "Remove answer"
  • Tap that and tap again on the confirmation.
  • You should be sent back to the My Prayers list and the answer to that prayer should be gone.

TODO

  • I am affirming this is my best work (Ecclesiastes 9:10)
  • PR has a relevant title that will be understandable in a public changelog (ie...non developers)
  • No new warnings
  • Upload GIF(s) of iOS and Android if applicable
  • Set a relevant reviewer

REVIEW

  • Review updates to test coverage and snapshots
  • Review code through the lens of being concise, simple, and well-documented

Manual QA

  • Manual QA on iOS and ensure it looks/behaves as expected
  • Manual QA on Android and ensure it looks/behaves as expected

The purpose of PR Review is to improve the quality of the software.

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #144 into master will increase coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   51.76%   51.82%   +0.05%     
==========================================
  Files         107      107              
  Lines        1418     1428      +10     
  Branches      355      358       +3     
==========================================
+ Hits          734      740       +6     
- Misses        607      611       +4     
  Partials       77       77              
Impacted Files Coverage Δ
...ayer/AnswerPrayerForm/AnswerPrayerFormConnected.js 52.38% <43.75%> (+2.38%) ⬆️
src/prayer/AnswerPrayerForm/AnswerPrayerForm.js 83.33% <100.00%> (+0.98%) ⬆️

@richarddubay richarddubay added the ready for review This one is ready to review label May 5, 2020
@redreceipt redreceipt added blocked Blocked by another PR and removed ready for review This one is ready to review labels May 6, 2020
@redreceipt
Copy link
Contributor

blocked on NewSpring/crete#95

@richarddubay richarddubay removed the blocked Blocked by another PR label May 13, 2020
@richarddubay
Copy link
Member Author

@redreceipt This is no longer blocked. Should be good to go.

Comment on lines 64 to 71
onClose={() => this.closePrayer()}
title={"Celebrate God's faithfulness"}
prayerId={this.prayerId}
prayerText={this.props.navigation.getParam(
'prayerText',
''
)}
prayerAnswer={this.prayerAnswer}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you're defining some of these as separate functions and variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I did that because I use prayerId and prayerAnswer a little further down. So instead of calling the getParam function again I just made one function for each of them that would set it.

richarddubay and others added 3 commits May 14, 2020 09:09
Co-authored-by: Michael Neeley <micneeley14@gmail.com>
Co-authored-by: Michael Neeley <micneeley14@gmail.com>
@richarddubay
Copy link
Member Author

@redreceipt Okay so I made everything use getParam now. But doesn't it feel odd to check this.props.navigation.getParam('prayerAnswer', '') to see if there is one?

@redreceipt
Copy link
Contributor

@redreceipt Okay so I made everything use getParam now. But doesn't it feel odd to check this.props.navigation.getParam('prayerAnswer', '') to see if there is one?

Yea you're right it does. What about defining:

prayer = {
  id: getParam(id),
  text: getParam(text),
  answer: getParam(answer),
}

and then using the prayer object everywhere:

<AddPrayerForm prayer={this.prayer} action={this.prayer.answer ? <Stuff /> : null} />

@richarddubay
Copy link
Member Author

Yeah ... actually that's much, much better.

Comment on lines +23 to +25
id: this.props.navigation.getParam('prayerId', ''),
text: this.props.navigation.getParam('prayerText', ''),
answer: this.props.navigation.getParam('prayerAnswer', ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't we pass these in as one combined nav param?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

prayer = this.props.navigation.getParam('prayer', {id: "", text: "", answer: ""})

@redreceipt redreceipt self-requested a review May 14, 2020 18:08
@redreceipt redreceipt added the merge When the tests pass, merge! label May 14, 2020
@mergify mergify bot merged commit 6d8c401 into master May 14, 2020
@mergify mergify bot deleted the remove-answer branch May 14, 2020 18:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge When the tests pass, merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants