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

Create email invite confirmation screen #215

Merged
merged 11 commits into from
Oct 19, 2023

Conversation

nathangong
Copy link
Contributor

Summary

Resolves #205

Creates a confirmation screen for email invite, access through /confirm-invite

Checklist

UI Requirements

- [ ] Information about the invitation (i.e., the author of the schedule, its name, and the account that is given access to this schedule) is displayed. -- Figma doesn't specify any of this information (?)

Functional Requirements

  • The screen automatically redirects after 30s.
  • Works for preview sites.

How to Test

Run it locally and navigate to http://localhost:3000/confirm-invite

@github-actions
Copy link

github-actions bot commented Mar 31, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://gt-scheduler.github.io/website/pr-preview/pr-215/
on branch gh-pages at 2023-10-19 17:41 UTC

@samarth52 samarth52 self-requested a review April 7, 2023 20:48
Copy link
Contributor

@samarth52 samarth52 left a comment

Choose a reason for hiding this comment

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

Nice work on this screen! Apart from the in-line comments, please address the following changes as well:

  • Integrate InviteBackLink and EmailInviteConfirmation into one component corresponding to the /invite/:id route. This will be used to integrate the Firebase function call that adds the friend
  • Implement a state that will be used to modify the contents displayed on the confirmation screen. There are three possible states: Loading, Success, Error (does not need to be this exactly)
  • When the user lands on this route, a loading message or symbol should be displayed until handleInvite is completed. Do not redirect or display the button that redirects.
  • If handleInvite is successful, display what is currently shown in EmailInviteConfirmation and redirect in 5s
  • If handleInvite fails, display what is currently shown in EmailInviteConfirmation but replace Congratulations on Adding a New Schedule to your View! with some apt error message. Redirect in 10s

Comment on lines 9 to 13
useEffect(() => {
setTimeout(() => {
navigate('/');
}, 30000);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change the timeout to 5s. Websites usually mention 30s in case the redirect fails or takes too much time, but they trigger the redirect much earlier on.

If you have not been redirected in 30 seconds, please click the button
below
</p>
<button type="button" className="continue-button">
Copy link
Contributor

Choose a reason for hiding this comment

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

The button is missing the on-click redirect functionality.

display: flex;
flex-direction: column;
align-items: center;
padding-top: 160px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this responsive. For larger screens, the gap appears to be too large:

image

flex-direction: column;
align-items: center;
padding-top: 160px;
padding-bottom: 38px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed since the footer is already absolutely positioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

This applies to line 13

Comment on lines 22 to 30
h1 {
font-size: 24px;
align-self: flex-start;
}

p {
align-self: flex-start;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The text should be center-aligned even in mobile view. So, this can be removed.

padding-left: 32px;
padding-right: 32px;
font-size: 24px;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a text-align: center; over here so all text is center aligned even when broken into multiple lines.

font-size: 24px;
height: 100%;

@media (max-width: 1000px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The page is responsive to the width but not the height. The button appears below the BoG text and logo:

Copy link
Contributor

@samarth52 samarth52 left a comment

Choose a reason for hiding this comment

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

Great work integrating my comments! There are just a few minor changes to look into. I would also recommend testing it locally using the Firebase emulator with the changes from Yatharth's Firebase functions PR: gt-scheduler/firebase-conf#3

src/components/InviteBackLink/index.tsx Outdated Show resolved Hide resolved
src/components/InviteBackLink/index.tsx Outdated Show resolved Hide resolved
src/components/InviteBackLink/stylesheet.scss Outdated Show resolved Hide resolved
src/components/InviteBackLink/index.tsx Outdated Show resolved Hide resolved
@samarth52
Copy link
Contributor

Lgtm 👍

@samarth52 samarth52 changed the base branch from bog-changes-s23 to bog-changes-s23-merged October 19, 2023 17:10
@samarth52 samarth52 merged commit 83f447e into bog-changes-s23-merged Oct 19, 2023
3 checks passed
@samarth52 samarth52 deleted the nathan/205-confirmation-screen branch October 19, 2023 17:46
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.

2 participants