Feat: Sign up prompt on hub pages - #1125
Open
JoshKornfeld wants to merge 12 commits into
Open
JoshKornfeld wants to merge 12 commits into
JoshKornfeld wants to merge 12 commits into
Conversation
…t/climateconnect into feat/sign-up-pop-up-prompt
JoshKornfeld
marked this pull request as ready for review
November 10, 2022 12:20
JoshKornfeld
requested review from
ddhanesha and
positiveimpact
as code owners
November 10, 2022 12:20
piperchester
reviewed
Dec 5, 2022
piperchester
left a comment
piperchester
left a comment
Contributor
There was a problem hiding this comment.
Awesome work so far!
Comment on lines
+334
to
+341
| buttonText={texts.join + "!"} | ||
| image={"/images/sign_up_prompt_image.jpg"} | ||
| infoTextOne={texts.receive_regular_updates_about_projects_topics} | ||
| infoTextTwo={texts.send_messages_comments_and_more} | ||
| onClose={handleCloseSignUpPrompt} | ||
| open={showSignUpPrompt} | ||
| subTitle={texts.sign_up_today_and_help_make_climate_neutral} | ||
| title={texts.change_the_world} |
Contributor
There was a problem hiding this comment.
Nit: do you mind sorting these props? If you use VS Code this should be really easy with cmd+shift+p and type 'alphabetize'.
| export default function Signup() { | ||
| const { ReactGA } = useContext(UserContext); | ||
| const router = useRouter(); | ||
| const email = router.query.email; |
Contributor
There was a problem hiding this comment.
Will .query always be defined? Want to minimize the chance for ReferenceError.
Comment on lines
+46
to
+58
| // wasn't really sure how to handle the first time a user comes the page as the cookie being looked up is undefined at first | ||
| if (typeof cookieContent === "undefined") { | ||
| const expiryDate = new Date(); | ||
| const month = (expiryDate.getMonth() + 1) % 12; | ||
| /* | ||
| new Date = gets today's date. Then, the next line is to get next month. | ||
| We use modulo for the edge case where it is currently December and we want it to return 1, instead of 13. | ||
| We then set variable, expiryDate (now one month ahead), to our cookie. */ | ||
| expiryDate.setMonth(month); | ||
| const cookieProps = getCookieProps(expiryDate); | ||
| cookies.set("display_signup_prompt", "true", cookieProps); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Do you mind writing a unit test for this behavior?
|
|
||
| export function getDisplaySignUpPromptFromCookie(cookieContent) { | ||
| const cookies = new Cookies(); | ||
| // wasn't really sure how to handle the first time a user comes the page as the cookie being looked up is undefined at first |
Contributor
There was a problem hiding this comment.
That's fair! We could probably remove this comment, or keep its context in the PR description as opposed to the code.
Suggested change
| // wasn't really sure how to handle the first time a user comes the page as the cookie being looked up is undefined at first |
| de: "Alle Orte", | ||
| }, | ||
| sign_up_today_and_help_make_climate_neutral: { | ||
| en: `Sign up today and help to make ${hubName} climate neutral.`, |
Contributor
There was a problem hiding this comment.
Honest question we might want to codify / standardize: do we always include punctuation in the translations? E.g., this .. I think it'd be useful to pick one way or the other: always include, or never include. WDYT?
Comment on lines
+91
to
+92
|
|
||
|
|
| width:"65%", | ||
| height:"65%", | ||
| [theme.breakpoints.down("sm")]: { | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #1120
Introduces a new component
SignUpPromptDialogaccording to the design in the issue. This prompt will show up to users who are not logged in and are viewing a location hub (Climate Hub Erlangen) and under the condition that they did not a set a cookie "display_signup_prompt".The user can close the dialog which will prevent it from being shown for 30 days or until cookies are cleared. The user can also click join to sign up or enter their email and then click join. Clicking join will redirect the user to the sign up page. The input of the textfield will be displayed in the email field of sign up.
Missing features / TODO:
Test plan
Before landing
yarn lintpasses (frontend)yarn formatpasses (frontend)make formatpasses (backend)