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.

Sign up for GitHub

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

Jump to bottom

Nomination page for epic's #316

Merged
merged 10 commits into from
Apr 18, 2021
Merged

Nomination page for epic's #316

merged 10 commits into from
Apr 18, 2021

Conversation

Delota
Copy link
Member

@Delota Delota commented Apr 12, 2021 •
edited
Loading

Files added:

  • RescueIdFieldset Field for validation of correct UUID
  • EpicNominationForm Wrapper that holds the RescueNominationForm and the RatNominationForm(coming later)
  • NominateRescueForm.js Holds the form and logic for calling the searchRescueApi and createEpicApi
  • RescueDetail.js Is loaded with rescue data when a rescue is found
  • useSearchRescueApi/useCreateEpicApi.js Wrappers for redux calls. Make sure that the Forms are not dealing with API directly4

Todo before merging:

  • Change regex to UUIDv4 validation
  • Create link to this page
  • Make sure FuelRatsApi allow epic creation for rats

To come after this PR in no particular order:

  • Page to nominate without a rescue(single rat)
  • Page for managing the epics
  • Page for redeeming your epic
Click for an impression

form3

@kenneaal
Copy link
Member

Looks good, stylelint errors should probably be fixed before PR is finalized. Is epic.write permission for regular rats the blocker at this point? @xlexi , do we perhaps need to break out a more granular permission set for this, so rats can nominate, but not otherwise change epic data?

@SuperManifolds
Copy link
Member

I plan to sort this issue with just an exception in the add endpoint requiring no additional permission to set these fields on creation, however I will be unable to make these changes until later this week.

@Delota Delota marked this pull request as ready for review April 14, 2021 17:17
@Delota Delota requested a review from UncleClapton April 14, 2021 17:18
@Delota Delota added the Type | Feature ✨ A PR that Introduces a new feature. label Apr 14, 2021
UncleClapton
UncleClapton requested changes Apr 15, 2021
View reviewed changes
Copy link
Member

@UncleClapton UncleClapton left a comment

Choose a reason for hiding this comment

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

Overall great stuff! I got both a few notes and stuff that needs fixing/talking about.

src/components/Forms/EpicNominationForm/EpicNominationForm.js Outdated
)

}
{type === 'RAT' && <p> {'Coming soon...'}</p>}
Copy link
Member

Choose a reason for hiding this comment

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

coming soon? 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's going to be the other form, which does require different handling, but the radiofieldset was there already 😆

Copy link
Member

Choose a reason for hiding this comment

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

Is there anything blocking that from being completed this PR?

UncleClapton
UncleClapton requested changes Apr 15, 2021
View reviewed changes
@Delota Delota requested a review from UncleClapton April 15, 2021 19:34
@Delota Delota force-pushed the feature/nomination-page branch from 762b7e0 to 4356c3e Compare April 15, 2021 19:38
UncleClapton
UncleClapton requested changes Apr 15, 2021
View reviewed changes
src/components/Forms/EpicNominationForm/EpicNominationForm.js Outdated
)

}
{type === 'RAT' && <p> {'Coming soon...'}</p>}
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything blocking that from being completed this PR?

@Delota
Copy link
Member Author

Delota commented Apr 16, 2021

@UncleClapton

Is there anything blocking that from being completed this PR?

This has now been added!

@Delota Delota requested a review from UncleClapton April 16, 2021 22:21
@Delota Delota force-pushed the feature/nomination-page branch from 46845ae to 7fe210b Compare April 18, 2021 20:48
@Delota Delota requested a review from UncleClapton April 18, 2021 20:52
UncleClapton
UncleClapton approved these changes Apr 18, 2021
View reviewed changes
Copy link
Member

@UncleClapton UncleClapton left a comment

Choose a reason for hiding this comment

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

LGTM!

@UncleClapton
Copy link
Member

@all-contributors please add @Delota for code

@allcontributors allcontributors bot mentioned this pull request Apr 18, 2021
Merged
@allcontributors
Copy link
Contributor

@UncleClapton

I've put up a pull request to add @Delota! 🎉

@UncleClapton UncleClapton merged commit b35bd64 into develop Apr 18, 2021
@UncleClapton UncleClapton deleted the feature/nomination-page branch April 18, 2021 20:58
UncleClapton added a commit that referenced this pull request May 17, 2021
@UncleClapton
docs(CHANGELOG.md): add changes for #316, #317, #320
fbeeee9
@UncleClapton UncleClapton mentioned this pull request May 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UncleClapton UncleClapton UncleClapton approved these changes

Assignees
No one assigned
Labels
Type | Feature ✨ A PR that Introduces a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Delota @kenneaal @SuperManifolds @UncleClapton