-
Notifications
You must be signed in to change notification settings - Fork 1
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
added in alert notifications #343
Conversation
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -111,6 +114,9 @@ const Register = (): JSX.Element => { | |||
router.push('/weeklyPicks'); | |||
} catch (error) { | |||
console.error('Registration Failed', error); | |||
toast.custom( | |||
<Alert variant={AlertVariants.Error} message="Something went wrong." />, | |||
); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a toast for successfully registering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it in, but when you register successfully it just redirects you to /weeklyPicks. I can find a way to add it in so it pops up when you're on the /weeklyPicks page after registering
app/weeklyPicks/WeeklyPicks.tsx
Outdated
@@ -100,6 +100,9 @@ const WeeklyPicks = ({ NFLTeams, currentGameWeek }: Props): JSX.Element => { | |||
|
|||
if (!gameGroupData || !weeklyPicksData) { | |||
console.error('Error getting game data'); | |||
toast.custom( | |||
<Alert variant={AlertVariants.Error} message="Something went wrong." />, | |||
); | |||
return; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious in line 174, could we have You picked {currentUserPick[user.id].team} was successful
=> You picked Falcons was successful
. Thoughts? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i definitely can! I'll give that a try so people know for sure that the system saw what they picked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to add unit test to ensure these notifications are firing when they should be.
components/Form/Form.tsx
Outdated
toast.custom( | ||
<Alert | ||
variant={AlertVariants.Error} | ||
message="You're missing information." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does this fire? It doesn't seem like the right message or even if it needs an alert.
… not seeing a value of having one there
context/AuthContextProvider.tsx
Outdated
@@ -60,7 +63,16 @@ export const AuthContextProvider = ({ | |||
await account.createEmailPasswordSession(user.email, user.password); | |||
getUser(); | |||
router.push('/weeklyPicks'); | |||
toast.custom( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit test for logging in to shoe the toast notification should go in this unit test file. There's none right now, so we'll need to create one. Only add test case for what you are touching though.
app/login/page.test.tsx
Outdated
} | ||
|
||
await mockLoginAccount(pretendUser); | ||
expect(account.createEmailPasswordSession).toBeInstanceOf(Object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason to test that this is an object. Also, createEmailPasswordSession is not a part of this file and doesn't need to be tested. Remove these test and test the alert logic in AuthContextProvider.
app/register/page.test.tsx
Outdated
@@ -19,6 +22,8 @@ const mockUseAuthContext = { | |||
isSignedIn: false, | |||
}; | |||
|
|||
account.create = jest.fn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we mocking this function?
app/login/page.test.tsx
Outdated
await mockLoginAccount(pretendUser); | ||
expect(account.createEmailPasswordSession).toBeInstanceOf(Object); | ||
|
||
render(<Alert variant={AlertVariants.Success} message="You've successfully logged in!" />) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this is not testing, this is executing the component. We want to test that the component is on the screen with the variant and message we expect. In line is executing the component onto the screen.
app/register/page.test.tsx
Outdated
@@ -102,4 +107,40 @@ describe('Register', () => { | |||
|
|||
mockUseAuthContext.isSignedIn = false; | |||
}); | |||
|
|||
test('successfully registers and shows success notification', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with these test. This is incorrectly tested. OnSubmit, it's either going to pass or fail. You need to mock the response of a success or failure. Then, the alert will show up within the component. Check that the alert is in the component and we're expecting the correct variant and message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of nit picks but if I don't call out the lack of alphabetically sorting props and such, Shashi will
app/register/page.test.tsx
Outdated
expect(toast.custom).toHaveBeenCalledWith( | ||
<Alert | ||
variant={AlertVariants.Error} | ||
message="Something went wrong!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort alphabetically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the need to not alphabetize this as I always define the type of variant first before providing a custom message. The message is always based on the variant type.
context/AuthContextProvider.test.tsx
Outdated
); | ||
}); | ||
|
||
test('should show error notification after a login attempt errors', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change verbiage to "... after a login attempt errors fails"
context/AuthContextProvider.tsx
Outdated
@@ -19,7 +20,7 @@ type UserCredentials = { | |||
type AuthContextType = { | |||
isSignedIn: boolean; | |||
setIsSignedIn: React.Dispatch<React.SetStateAction<boolean>>; | |||
loginAccount: (user: UserCredentials) => Promise<void | Error>; // eslint-disable-line no-unused-vars | |||
login: (user: UserCredentials) => Promise<void | Error>; // eslint-disable-line no-unused-vars | |||
logoutAccount: () => Promise<void>; | |||
getUser: () => Promise<IUser | undefined>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort alphabetically
context/AuthContextProvider.tsx
Outdated
@@ -123,7 +122,7 @@ export const AuthContextProvider = ({ | |||
() => ({ | |||
isSignedIn, | |||
setIsSignedIn, | |||
loginAccount, | |||
login, | |||
logoutAccount, | |||
getUser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort alphabetically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix: #337
Integrate toast notifications into our site's user interface to enhance user experience by providing clear and timely feedback for important actions and events.
Acceptance Criteria: