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

Feature/49 sign up verification #100

Merged
merged 15 commits into from
Oct 25, 2022
Merged

Conversation

craineum
Copy link
Contributor

Pull Request

Story

Fix #49

Description

Add sign up email verification

Screenshots

Affected Areas

List out the areas affected by your code change.

Tests

Describe the tests.

Was this feature tested on all browsers?

  • Chrome
  • Firefox
  • Edge
  • Internet Explorer
  • Safari

Related PRs

Add any related pull requests

Definition of Done

  • README updated
  • Tests written
  • Acceptance criteria implement
  • Code reviewed
  • Feature accepted

PR Comments Emoji Legend

😻 - :heart_cat_eyes: Compliment to code/idea/etc
♻️ - :recycle: Non-blocking proposed refactor
➕ - :heavy_plus_sign: Non-blocking proposed addition
🔥 - :fire: Non-blocking proposed removal
❓ - :question: Non-blocking question
⚠️ - :warning: Blocking comment that must be addressed before PR

Gifs for life (required)

mandatory

@craineum craineum force-pushed the feature/49-sign-up-verification branch from 67233ac to 7568ea2 Compare October 21, 2022 17:34
@archae0pteryx archae0pteryx temporarily deployed to redwood-temp-app-pr-100 October 21, 2022 17:38 Inactive
@archae0pteryx archae0pteryx temporarily deployed to redwood-temp-app-pr-100 October 24, 2022 18:33 Inactive
@archae0pteryx archae0pteryx temporarily deployed to redwood-temp-app-pr-100 October 24, 2022 19:25 Inactive
@craineum craineum force-pushed the feature/49-sign-up-verification branch from de8e740 to b978952 Compare October 24, 2022 19:44
@archae0pteryx archae0pteryx temporarily deployed to redwood-temp-app-pr-100 October 24, 2022 19:44 Inactive
Copy link
Contributor

@archae0pteryx archae0pteryx left a comment

Choose a reason for hiding this comment

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

LGTM!
just an aside, you can do user?.thing now instead of user && user.thing
also, do you think it might be worthwhile to create a mapping of response errors?

const API_ERRORS = {
  EXPECTED_SOMETIMES_ERROR: "Sometimes this will happen",
  ...
}

So that we're not matching response.error === "i didnt do something" like this (potentially in multiple places), instead matching response.error === API_ERRORS.EXPECTED_SOMETIMES... . I mention it cause it's bitten me in the past - I've had a few components matching the same string for response errors... Probably overkill at this time but thought i'd throw it out there since i saw that here.

@craineum
Copy link
Contributor Author

@archae0pteryx
Thanks for the feedback. I had thought about coming back around and figuring out how to share data between the api and the web for things like error mapping, but completely forgot, so thanks for the reminder!

@archae0pteryx archae0pteryx temporarily deployed to redwood-temp-app-pr-100 October 25, 2022 12:00 Inactive
@craineum
Copy link
Contributor Author

Opened a new Issue #104 for handling the error response messages between api and web sides.

@craineum craineum merged commit 0db35d6 into main Oct 25, 2022
@craineum craineum deleted the feature/49-sign-up-verification branch October 25, 2022 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Sign-Up email verification
4 participants