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

ryan/feat(ux: create input loading spinner) #399

Merged
merged 49 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
fe61aee
feat(LoadingSpinner.tsx): created first iteration of a loading spinner
ryandotfurrer Jul 5, 2024
5d7fb40
feat(LoadingSpinner.tsx): added optional height and width props to co…
ryandotfurrer Jul 13, 2024
86c9e36
feat(LoadingSpinner.tsx): add accessibility features
ryandotfurrer Jul 13, 2024
f69924e
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 13, 2024
ba83fee
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 16, 2024
879e45d
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 16, 2024
0642af5
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 16, 2024
9fe6c06
Merge remote-tracking branch 'origin/develop' into ryan/create-input-…
ryandotfurrer Jul 17, 2024
cfbb118
Merge branch 'ryan/create-input-loading-spinner' of https://github.co…
ryandotfurrer Jul 17, 2024
91a1cad
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 19, 2024
3ae4268
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 19, 2024
8555c8c
feat(page.tsx): remove loading spinner component from league/all page
ryandotfurrer Jul 19, 2024
d1ea90e
feat(LoadingSpinner.tsx): made loadingspinner respect min and max hei…
ryandotfurrer Jul 19, 2024
0a97191
fix(LeagueCard.tsx): remove hard height on leaguecard component
ryandotfurrer Jul 19, 2024
0a17029
test(LoadingSpinner.test.tsx): create test for loadingspinner component
ryandotfurrer Jul 19, 2024
ddf61e9
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 23, 2024
b13771f
fix(LeagueCard.tsx): added hover styles that were unintentionally rem…
ryandotfurrer Jul 23, 2024
7405e32
fix(league/all/page.tsx): revereted loading text value back so the te…
ryandotfurrer Jul 23, 2024
f951b9f
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 25, 2024
896684d
fix(/login/page.tsx): added router import
ryandotfurrer Jul 26, 2024
ada9f2a
test(LoadingSpinner.test.tsx): test only the loadingspinner component…
ryandotfurrer Jul 27, 2024
f4f6297
fix(register/page.tsx-and-login/page.tsx): updated userouter import t…
ryandotfurrer Jul 27, 2024
10d8372
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 30, 2024
6750360
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Jul 31, 2024
6e05eee
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Aug 8, 2024
a5ff75e
feat(loadingspinner.tsx): move data-testid to loadingspinner containi…
ryandotfurrer Aug 9, 2024
c57250a
test(LoadingSpinner.test.tsx): remove line checking for classes as I …
ryandotfurrer Aug 9, 2024
a09b347
Merge branch 'develop' of https://github.com/LetsGetTechnical/gridiro…
ryandotfurrer Aug 13, 2024
3691ac6
change buttonprops label type and remove react from loadingspinner fo…
ryandotfurrer Aug 13, 2024
2fcd56a
(button.tsx): change previous change on buttonprops label to a string…
ryandotfurrer Aug 13, 2024
f8159f1
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Aug 14, 2024
5bbb240
sort imports in login page.tsx
ryandotfurrer Aug 15, 2024
045fea2
attempt at adding test to login page for loading spinner
ryandotfurrer Aug 16, 2024
6a45440
feat(login/page.tsx): add try and catch statements to onSubmit function
ryandotfurrer Aug 16, 2024
efbf890
attempt to make loadingspinner test work
ryandotfurrer Aug 16, 2024
4b7be56
implemented shashi's testing logic for the loadingspinner on the logi…
ryandotfurrer Aug 21, 2024
dd0f185
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Aug 21, 2024
bbce891
Clue355/feat(#449): Use input spinner for submission buttons (#453)
Clue355 Aug 26, 2024
42f4023
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Aug 29, 2024
0803b5a
address comments in latest PR review including updating the login pag…
ryandotfurrer Aug 29, 2024
1a86fce
fix build error pertaining to the new throw in login page.tsx
ryandotfurrer Aug 29, 2024
68f5ac5
remove redundant setIsLoading(false) from try statement
ryandotfurrer Aug 30, 2024
edf7ccb
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Sep 5, 2024
585c807
add `finally` statement to register page `onSubmit` function
ryandotfurrer Sep 5, 2024
564e8c4
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Sep 5, 2024
cd6711d
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Sep 19, 2024
6beef05
update login/page.test.tsx tests
ryandotfurrer Sep 19, 2024
a81040b
fix register page tests
ryandotfurrer Sep 20, 2024
f910e60
Merge branch 'develop' into ryan/create-input-loading-spinner
ryandotfurrer Sep 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions app/(main)/login/page.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
import React from 'react';
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
import React, { Dispatch, useState as useStateMock } from 'react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
import Login from './page';
import { loginAccount } from '@/api/apiFunctions';

const mockLogin = jest.fn();
const mockPush = jest.fn();
const getUser = jest.fn();
const setIsLoading = jest.fn();
const setState = jest.fn();

jest.mock('react', () => ({
...jest.requireActual('react'),
useState: jest.fn(),
}));

let continueButton: HTMLElement,
emailInput: HTMLInputElement,
Expand Down Expand Up @@ -33,10 +41,17 @@ jest.mock('../../../context/AuthContextProvider', () => ({
}));

describe('Login', () => {
const setIsLoading = jest.fn();
const useStateMock = (init: any): [unknown, Dispatch<unknown>] => [
init,
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
setState,
];
beforeEach(() => {
jest.clearAllMocks();
jest.spyOn(React, 'useState').mockImplementation(useStateMock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not return an error in your local machine? I tried using this syntax for my loadingSpinner test, but it kept returning an error for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@choir27 it does; and it did before. I just haven't been able to get back to this one :P

Copy link
Member Author

Choose a reason for hiding this comment

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

@choir27 fixed


render(<Login />);

emailInput = screen.getByTestId('email');
passwordInput = screen.getByTestId('password');
continueButton = screen.getByTestId('continue-button');
Expand All @@ -60,9 +75,10 @@ describe('Login', () => {
test('should call loginAccount function with email and password when continue button is clicked', async () => {
fireEvent.change(emailInput, { target: { value: '[email protected]' } });
fireEvent.change(passwordInput, { target: { value: 'password123' } });
fireEvent.click(continueButton);
fireEvent.submit(continueButton);

await waitFor(() => {
expect(mockLogin).toHaveBeenCalledTimes(1);
expect(mockLogin).toHaveBeenCalledWith({
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
email: '[email protected]',
password: 'password123',
Expand All @@ -87,3 +103,30 @@ describe('Login', () => {
expect(mockPush).toHaveBeenCalledWith('/league/all');
});
});

describe('Login loading spinner', () => {
it('should show the loading spinner', async () => {
(useStateMock as jest.Mock).mockImplementation((init: boolean) => [
true,
setIsLoading,
]);
Comment on lines +108 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

Your tests seem to be failing, please look into this and make sure it's changing the loading state properly

Copy link
Member Author

Choose a reason for hiding this comment

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

@choir27 I was aware :P Just been busy working on other things and unable to get back to this. TY!

Copy link
Member Author

Choose a reason for hiding this comment

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

@choir27 fixed


render(<Login />);

await waitFor(() => {
expect(screen.queryByTestId('loading-spinner')).toBeInTheDocument();
});
});
it('should not show the loading spinner', async () => {
(useStateMock as jest.Mock).mockImplementation((init: boolean) => [
false,
setIsLoading,
]);

render(<Login />);

await waitFor(() => {
expect(screen.queryByTestId('loading-spinner')).not.toBeInTheDocument();
});
});
});
25 changes: 17 additions & 8 deletions app/(main)/login/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import {
} from '../../../components/Form/Form';
import { Input } from '@/components/Input/Input';
import { useAuthContext } from '@/context/AuthContextProvider';
import { useRouter } from 'next/navigation';
import { z } from 'zod';
import { zodResolver } from '@hookform/resolvers/zod';
import LinkCustom from '@/components/LinkCustom/LinkCustom';
import LoadingSpinner from '@/components/LoadingSpinner/LoadingSpinner';
import Logo from '@/components/Logo/Logo';
import logo from '@/public/assets/logo-colored-outline.svg';
import React, { useEffect, useState } from 'react';
import { useRouter } from 'next/navigation';

/**
* The schema for the login form.
Expand Down Expand Up @@ -82,10 +82,19 @@ const Login = (): React.JSX.Element => {
* Handles the form submission.
* @param {LoginUserSchemaType} data - The data from the form.
*/
const onSubmit: SubmitHandler<LoginUserSchemaType> = async (data) => {
setIsLoading(true);
await login(data);
setIsLoading(false);
const onSubmit: SubmitHandler<LoginUserSchemaType> = async (
data: LoginUserSchemaType,
): Promise<void> => {
try {
setIsLoading(true);
await login(data);
setIsLoading(false);
chris-nowicki marked this conversation as resolved.
Show resolved Hide resolved
} catch (error) {
console.error(error);
throw new Error('An error occurred while logging in');
} finally {
setIsLoading(false);
}
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of setting setIsLoading(false) after the API or error, use finally. Then you'd only have to set it once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh so smart, thank you! This will be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in latest commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shashilo pinging you for a reminder, please advise.

};

return (
Expand Down Expand Up @@ -130,7 +139,7 @@ const Login = (): React.JSX.Element => {
{...field}
/>
</FormControl>
{form.formState.errors.email && (
{form.formState.errors?.email && (
<FormMessage>
{form.formState.errors.email.message}
</FormMessage>
Expand All @@ -151,9 +160,9 @@ const Login = (): React.JSX.Element => {
{...field}
/>
</FormControl>
{form.formState.errors.password && (
{form.formState.errors?.password && (
<FormMessage>
{form.formState.errors.password.message}
{form.formState.errors?.password.message}
</FormMessage>
)}
</FormItem>
Expand Down
48 changes: 43 additions & 5 deletions app/(main)/register/page.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
import {
render,
screen,
fireEvent,
waitFor,
} from '@testing-library/react';
import React, { Dispatch, useState as useStateMock } from 'react';
import Register from './page';
import { registerAccount } from '@/api/apiFunctions';
import Alert from '@/components/AlertNotification/AlertNotification';
Expand All @@ -7,8 +13,15 @@ import { toast } from 'react-hot-toast';

const mockLogin = jest.fn();
const mockPush = jest.fn();
const setIsLoading = jest.fn();
const setState = jest.fn();

jest.mock('../../../api/apiFunctions', () => ({
jest.mock('react', () => ({
...jest.requireActual('react'),
useState: jest.fn()
}));

jest.mock('@/api/apiFunctions', () => ({
registerAccount: jest.fn(),
}));

Expand Down Expand Up @@ -44,8 +57,11 @@ jest.mock('../../../context/AuthContextProvider', () => ({
}));

describe('Register', () => {
const setIsLoading = jest.fn();
const useStateMock = (init: any): [unknown, Dispatch<unknown>] => [init, setState];
beforeEach(() => {
jest.clearAllMocks();
jest.spyOn(React, 'useState').mockImplementation(useStateMock);

render(<Register />);

Expand Down Expand Up @@ -84,9 +100,10 @@ describe('Register', () => {
fireEvent.change(emailInput, { target: { value: '[email protected]' } });
fireEvent.change(passwordInput, { target: { value: 'rawr123' } });
fireEvent.change(confirmPasswordInput, { target: { value: 'rawr123' } });
fireEvent.click(continueButton);
fireEvent.submit(continueButton);

await waitFor(() => {
expect(registerAccount).toHaveBeenCalledTimes(1);
expect(registerAccount).toHaveBeenCalledWith({
email: '[email protected]',
password: 'rawr123',
Expand Down Expand Up @@ -116,7 +133,7 @@ describe('Register', () => {
fireEvent.change(emailInput, { target: { value: '[email protected]' } });
fireEvent.change(passwordInput, { target: { value: 'pw1234' } });
fireEvent.change(confirmPasswordInput, { target: { value: 'pw1234' } });
fireEvent.click(continueButton);
fireEvent.submit(continueButton);

await waitFor(() => {
expect(registerAccount).toHaveBeenCalledWith({
Expand Down Expand Up @@ -147,7 +164,7 @@ describe('Register', () => {
fireEvent.change(emailInput, { target: { value: '[email protected]' } });
fireEvent.change(passwordInput, { target: { value: 'pw1234' } });
fireEvent.change(confirmPasswordInput, { target: { value: 'pw1234' } });
fireEvent.click(continueButton);
fireEvent.submit(continueButton);

await waitFor(() => {
expect(registerAccount).toHaveBeenCalledWith({
Expand All @@ -171,3 +188,24 @@ describe('Register', () => {
expect(darkModeSection).toHaveClass('dark:bg-gradient-to-b');
});
});

describe('Register loading spinner', () => {
it('should show the loading spinner', async () => {
(useStateMock as jest.Mock).mockImplementation((init: boolean) => [true, setIsLoading]);

render(<Register />);

await waitFor(() => {
expect(screen.queryByTestId('loading-spinner')).toBeInTheDocument();
});
});
it('should not show the loading spinner', async () => {
(useStateMock as jest.Mock).mockImplementation((init: boolean) => [false, setIsLoading]);

render(<Register />);

await waitFor(() => {
expect(screen.queryByTestId('loading-spinner')).not.toBeInTheDocument();
});
});
});
21 changes: 13 additions & 8 deletions app/(main)/register/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import Alert from '@/components/AlertNotification/AlertNotification';
import LinkCustom from '@/components/LinkCustom/LinkCustom';
import Logo from '@/components/Logo/Logo';
import logo from '/public/assets/logo-colored-outline.svg';
import React, { JSX, useEffect } from 'react';
import React, { JSX, useEffect, useState } from 'react';
import LoadingSpinner from '@/components/LoadingSpinner/LoadingSpinner';

const RegisterUserSchema = z
.object({
Expand Down Expand Up @@ -54,6 +55,7 @@ type RegisterUserSchemaType = z.infer<typeof RegisterUserSchema>;
const Register = (): JSX.Element => {
const router = useRouter();
const { login, isSignedIn } = useAuthContext();
const [isLoading, setIsLoading] = useState(false);

useEffect(() => {
if (isSignedIn) {
Expand Down Expand Up @@ -105,8 +107,10 @@ const Register = (): JSX.Element => {
data: RegisterUserSchemaType,
): Promise<void> => {
try {
setIsLoading(true);
await registerAccount(data);
await login(data);
setIsLoading(false);
router.push('/league/all');
toast.custom(
<Alert
Expand All @@ -115,6 +119,7 @@ const Register = (): JSX.Element => {
/>,
);
} catch (error) {
setIsLoading(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why setIsLoading(false) is used twice? One in the try part and in the catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please explain why setIsLoading(false) is used twice? One in the try part and in the catch.

@vmaineng this was from @Clue355's branch of this branch that was approved and merged in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though I'd like to make this as similar to my code on the login page as possible.

@shashilo @Clue355 @vmaineng @chris-nowicki thoughts on me changing the onSubmit on the register/page.tsx to be inline with the one on the login/page.tsx?

Current Login onSubmit

const onSubmit: SubmitHandler<LoginUserSchemaType> = async (
    data: LoginUserSchemaType,
  ): Promise<void> => {
    try {
      setIsLoading(true);
      await login(data);
    } catch (error) {
      console.error(error);
      throw new Error('An error occurred while logging in');
    } finally {
      setIsLoading(false);
    }
  };

Current Register onSubmit

const onSubmit: SubmitHandler<RegisterUserSchemaType> = async (
    data: RegisterUserSchemaType,
  ): Promise<void> => {
    try {
      setIsLoading(true);
      await registerAccount(data);
      await login(data);
      setIsLoading(false);
      router.push('/league/all');
      toast.custom(
        <Alert
          variant={AlertVariants.Success}
          message="You have successfully registered your account."
        />,
      );
    } catch (error) {
      setIsLoading(false);
      console.error('Registration Failed', error);
      toast.custom(
        <Alert variant={AlertVariants.Error} message="Something went wrong!" />,
      );
    }
  };

Copy link
Member Author

Choose a reason for hiding this comment

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

@shashilo @Clue355 @vmaineng @chris-nowicki Please advise on the comment above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryandotfurrer in what ways to you plan to make it inline? by adding a finally to the register try/catch?

Copy link
Member Author

@ryandotfurrer ryandotfurrer Sep 4, 2024

Choose a reason for hiding this comment

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

@ryandotfurrer in what ways to you plan to make it inline? by adding a finally to the register try/catch?

@chris-nowicki precisely. Just to make them consistent. @vmaineng

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use finally. It allows you to only call the setIsLoading false once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Use finally. It allows you to only call the setIsLoading false once.

@shashilo added in the latest commit

console.error('Registration Failed', error);
toast.custom(
<Alert variant={AlertVariants.Error} message="Something went wrong!" />,
Expand Down Expand Up @@ -169,9 +174,9 @@ const Register = (): JSX.Element => {
{...field}
/>
</FormControl>
{form.formState.errors.email && (
{form.formState.errors?.email && (
<FormMessage>
{form.formState.errors.email.message}
{form.formState.errors?.email.message}
</FormMessage>
)}
</FormItem>
Expand All @@ -190,9 +195,9 @@ const Register = (): JSX.Element => {
{...field}
/>
</FormControl>
{form.formState.errors.password && (
{form.formState.errors?.password && (
<FormMessage>
{form.formState.errors.password.message}
{form.formState.errors?.password.message}
</FormMessage>
)}
</FormItem>
Expand All @@ -211,9 +216,9 @@ const Register = (): JSX.Element => {
{...field}
/>
</FormControl>
{form.formState.errors.confirmPassword && (
{form.formState.errors?.confirmPassword && (
<FormMessage>
{form.formState.errors.confirmPassword.message}
{form.formState.errors?.confirmPassword.message}
</FormMessage>
)}
</FormItem>
Expand All @@ -222,7 +227,7 @@ const Register = (): JSX.Element => {

<Button
data-testid="continue-button"
label="Register"
label={isLoading ? <LoadingSpinner /> : 'Continue'}
type="submit"
disabled={isDisabled}
/>
Expand Down
2 changes: 1 addition & 1 deletion components/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const buttonVariants = cva(
export interface ButtonProps
extends React.ButtonHTMLAttributes<HTMLButtonElement>,
VariantProps<typeof buttonVariants> {
label?: React.ReactNode;
label?: boolean | JSX.Element | string;
icon?: JSX.Element;
vmaineng marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down