email confirmation message banner #3576
Conversation
artsy-peril
bot
added
the
Version: Patch
|
@erikdstock in the future, let's always request review from at least some members of product-auctions so that there's shared visibility on our team's work. I'll review now; also requesting review from @yuki24 |
erikdstock
requested review from
dleve123 and
yuki24
and removed request for
mzikherman
| already_confirmed: "You have already confirmed your email.", | ||
| invalid_token: "Your token is invalid. Please try again.", | ||
| blank_token: "No token found. Please try again.", | ||
| expired_token: "An error has occurred. Please try again.", |
There was a problem hiding this comment.
Shouldn't this be something about how their confirmation token expired, not the general message? Or are we planning to come back to this once we allow people to request the email?
There was a problem hiding this comment.
I copied these from the eigen pr- happy to change but also wonder if these are meant to be final - in most cases for example 'try again' is not going to be actionable unless there is a 'resend email' button.
There was a problem hiding this comment.
Agreed – would much rather tell people to "Contact support@artsymail.com"
| name="close" | ||
| color={this.props.textColor} | ||
| fontSize="16px" | ||
| style={{ cursor: "pointer" }} |
There was a problem hiding this comment.
This and the other change to the CtaBanner story were minor bugfixes left over from early explorations. It's possible we don't even use this and/or should remove the changes from the PR but for now I'm thinking we want to prioritize upcoming reaction migration work so am not addressing unless you think it is neccessary @damassi
| /* email confirmation token was expired */ | ||
| | "expired_token" | ||
|
|
||
| type BannerMessage = string | React.FC |
There was a problem hiding this comment.
why the union type here? Seems like these are all strings?
| Message && ( | ||
| <Banner dismissable backgroundColor="black100"> | ||
| <Sans color="white100" size="3" lineHeight={1} weight="medium"> | ||
| {typeof Message === "string" ? Message : <Message />} |
There was a problem hiding this comment.
very well could be missing something, but these seem to always be strings...
|
approving to unblock merge, but still not sure what's up with the type complexity. |
|
🚀 PR was released in |
…to retire-reaction * 'retire-reaction' of https://github.com/damassi/force: (23 commits) Disable codecov for jest for now [Migration] Comment out experiment viewed [Migration] Fix tracking event invocation Move artsy/reaction#3590 [Migration] Update coffeescript /v2 paths to relative Move artsy/reaction#3587 Move artsy/reaction#3589 Move artsy/reaction#3576 Move artsy/reaction#3572 Rename .babelrc to babel.config.js [Migration] Finish migration [Migration] Migrate tests [Migration] Rename v2/*.test to .jest [Migration] Delete publishing folder [Migration] Update import paths [Migration] Enable incremental type-checking [Migration] Temporarily disable @types for relay [Migration] Storybooks [Migration] Add relay Fix type-check errors in force ...
Related 🔒 Jira Ticket
Low-risk code, so I will merge with one approving review.
This PR adds a simple confirmation message banner for use based on a query parameter in the url on force. Messages are currently hardcoded based on the incoming param - no reliance on the user being present.
Paired with @bhoggard with some input from @yuki24 on this.
Example linked into force (artsy/force#5620):
