-
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
ryan/feat(ux: create input loading spinner) #399
Changes from 9 commits
fe61aee
5d7fb40
86c9e36
f69924e
ba83fee
879e45d
0642af5
9fe6c06
cfbb118
91a1cad
3ae4268
8555c8c
d1ea90e
0a97191
0a17029
ddf61e9
b13771f
7405e32
f951b9f
896684d
ada9f2a
f4f6297
10d8372
6750360
6e05eee
a5ff75e
c57250a
a09b347
3691ac6
2fcd56a
f8159f1
5bbb240
045fea2
6a45440
efbf890
4b7be56
dd0f185
bbce891
42f4023
0803b5a
1a86fce
68f5ac5
edf7ccb
585c807
564e8c4
cd6711d
6beef05
a81040b
f910e60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ import { LeagueCard } from '@/components/LeagueCard/LeagueCard'; | |
import { ILeague } from '@/api/apiFunctions.interface'; | ||
import { getUserLeagues } from '@/utils/utils'; | ||
import { useDataStore } from '@/store/dataStore'; | ||
import { getGameWeek } from '@/api/apiFunctions'; | ||
import LoadingSpinner from '@/components/LoadingSpinner/LoadingSpinner'; | ||
import { ENTRY_URL, LEAGUE_URL } from '@/const/global'; | ||
|
||
/** | ||
|
@@ -56,9 +58,7 @@ const Leagues = (): JSX.Element => { | |
)) | ||
) : ( | ||
<div className="text-center"> | ||
<p className="text-lg font-bold"> | ||
You are not enrolled in any leagues | ||
</p> | ||
<LoadingSpinner height="max-h-32 h-32" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this max and hard set height? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added props to the loader so you can set the height of it to match the element it's replacing. I'll revise this so it takes up 100% of the height and width of what it's replacing by default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo This is outdated, can it be resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo pinging you for a reminder, please advise. |
||
</div> | ||
)} | ||
</section> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ const LeagueCard = React.forwardRef<HTMLAnchorElement, ILeagueCardProps>( | |
data-testid="LeagueCard" | ||
href={href} | ||
className={cn( | ||
'LeagueCard flex max-h-32 place-items-center gap-6 rounded-lg border bg-card p-4 text-card-foreground shadow-sm dark:border-zinc-800 hover:bg-zinc-800 transition', | ||
'LeagueCard flex max-h-32 h-32 place-items-center gap-6 rounded-lg border bg-card p-4 text-card-foreground shadow-sm dark:border-zinc-800', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DO NOT set a hard height on this component. This will remove its flexibility and create bugs when content overflows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo This is outdated, can it be resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo pinging you for a reminder, please advise. |
||
)} | ||
ref={ref} | ||
> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright (c) Gridiron Survivor. | ||
// Licensed under the MIT License. | ||
|
||
import React, { JSX } from 'react'; | ||
import { cn } from '@/utils/utils'; | ||
|
||
export interface ILoadingSpinnerProps { | ||
height?: string; | ||
width?: string; | ||
} | ||
/** | ||
* Renders a loading spinner. | ||
* @param {ILoadingSpinnerProps} props - The props for the loading spinner component. | ||
* @param {string} props.height - The height of the loading spinner. This should be the same tailwind class as the element it is standing in for. | ||
* @param {string} props.width - The width of the loading spinner. This should be the same tailwind class as the element it is standing in for. | ||
* @returns {JSX.Element} The rendered loading spinner component. | ||
*/ | ||
const LoadingSpinner = ({ | ||
height, | ||
width, | ||
}: ILoadingSpinnerProps): JSX.Element => { | ||
return ( | ||
<div | ||
role="alert" | ||
tabIndex={-1} | ||
className={cn( | ||
'loading-spinner-container w-full h-full place-content-center text-center', | ||
height, | ||
width, | ||
)} | ||
> | ||
<div | ||
className="loading-spinner w-12 h-12 mx-auto rounded-full border-4 border-t-muted-foreground border-foreground animate-spin motion-reduces:hidden" | ||
data-testid="loading-spinner" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this data-testid to the container There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in latest commit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo This is outdated, can it be resolved? |
||
/> | ||
<p className="sr-only motion-reduce:not-sr-only">Loading</p> | ||
</div> | ||
); | ||
}; | ||
|
||
export default LoadingSpinner; |
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.
This is incorrect. The loader should replace to submit content as the API is waiting for a response. This message is correct when there are 0 leagues associated with the user.
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.
Removed from this page
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 reverted the
back, but the styles were not carried over.
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.
@shashilo what loader is meant to be used on the LeagueCard component? I thought this input spinner was for buttons only?
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.
@ryandotfurrer Was this question addressed?
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.
<p className="text-lg font-bold">
These classes were removed. Wondering if this is intentional or not.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.
It was not. @shashilo, what loader is meant to be used on the LeagueCard component? I thought this input spinner was for buttons only?
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.
These are back in and will be reflected upon the next commit.
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.
@shashilo pinging you for a reminder, please advise.