-
Notifications
You must be signed in to change notification settings - Fork 0
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
#95 empty state search #96
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
FIYE we needed this. I was thinking of doing it in the loading.tsx
file (there's a loading skeleton ticket buried
somewhere on that board) but I think this is a better approach!
Edit: Found it! #51
@@ -106,6 +108,8 @@ export default function Roles({ | |||
</div> | |||
</div> | |||
)} | |||
{reviews.isSuccess && reviews.data.length === 0 && <NoResults />} |
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.
Totally a nit but thoughts on doing this above the "main" return
statement?
if (review.isPending) return <LoadingResult />
if (reviews.isSuccess && !reviews.data.length) return <NoResult />
So then you can remove all of the checks from Line 85.
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.
beautiful
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.
ok so i lied, if we do this then the search bar isn't rendered and adding it to the above return statements duplicates like an extra 15 lines of code so I think it might be better to leave where they are?
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're right i didn't realize that this was a page and that we have the SearchFilter
component on it
<Button | ||
type="button" | ||
variant="ghost" | ||
onClick={clearFilters} | ||
className="text-cooper-blue-600" | ||
> | ||
Clear Filters | ||
</Button> |
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 button might not be centered. See this
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.
def not xD
Description
Adds super simple (ugly even) loading and no-results components.
Motivation and Context
So this was j bothering me, like we need to do actual designs but it was low effort so I figured why not.
Also changed the cooper logo to be a component bc i was tired of figuring out the ratio of height to width or whatever
Closes #95
How has this been tested?
You may be thinking, "Wow, Cooper has made so much progress this semester! But I wonder where there tests are?". You no longer need to fret! See, although I didn't write any tests yet, I think adding tests is a great ✨ winter break activity ✨. So you can rest assured that even though there aren't any yet, a soon-to-be PR will fill that gap and mend your worries ◡̈ .
Screenshots (if appropriate):
Types of changes
Checklist: