[SSF-206] Add empty state components for dashboard#181
Conversation
jxuistrying
changed the title
jxuistrying
changed the title
| </Box> | ||
| {isPageEmpty ? ( | ||
| <PageEmptyState | ||
| subtitle="You have no orders or applications to review at this time." |
There was a problem hiding this comment.
Not sure if this is strictly necessary, but it might be good to pass in the entity to PageEmptyState and SectionEmptyState instead of hard coding the entire subtitle.
e.g. You have no {entity} at this time.
There was a problem hiding this comment.
im liking the sound of dis
There was a problem hiding this comment.
gotchu I added {entity} so that the content can be passed in
There was a problem hiding this comment.
awesome job implementing this! tested pages & overall a great first pr ><
commented on matching the figma, edge cases, and frontend styling!
take a look at apps/frontend/src/theme.ts for our repo design system
- specifically we use
<Text textStyle="p2 (...etc)"> and <Heading textStyle="h1 (...etc)">instead of hardcoding fontSize, fontWeight, fontFamily - we also have color tokens there to use instead of hardcoding hex numbers
- we have our font tokens there wired so we also don't have to hardcode those
can you update the styling to follow our conventions? ik seems tomato tomato but routing all styling through one source makes the app easier to maintain, extend, and look at :)
|
@jxuistrying btw - justin's FM dashboard pr has been merged! if you could add the empty state for FM dashboards, that would be so so great! |
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
i noticed a same fail-fast issue with using Promise.all so i suggested two fixes!
| /> | ||
| )} | ||
|
|
||
| {Object.values(DonationStatus).map((status) => { |
There was a problem hiding this comment.
can we add the same loading fix to this file as well?
instead of directly rendering empty state when it could be loading..
| useEffect(() => { | ||
| const fetchDashboardData = async () => { | ||
| try { | ||
| const [user, applications, allOrders, allDonations] = await Promise.all( |
There was a problem hiding this comment.
consider Promise.allSettled instead of Promise.all here~
Promise.all -
all the calls are bundled into one block, so if any single endpoint fails, the entire dashboard renders empty + a generic error, even if the other endpoints succeeded
Promise.allSettled -
lets each section render independently, so a failure in one call will not affect another
There was a problem hiding this comment.
OR actually consider...
so the problem is that promise.all is fail fast, so if any one endpoint rejects we skip all the set calls, even for sections that succeeded. this results in:
- one flaky call emptying the whole dashboard
- after the catch, loading is false but the arrays are still empty [], so isPageEmpty is true & we render empty state error showing that you have no orders" which defeats the empty-state feature
i suggest keeping the per section functions (each with its own try/catch) and just coordinating them for the loading flag:
useEffect(() => {
const load = async () => {
try {
await Promise.all([fetchMe(), fetchPendingApplications(), fetchRecentOrders(), fetchRecentDonations()]);
} finally {
setLoading(false);
}
};
load();
}, [setAlertMessage]);
each function catches its own error, so Promise.all never rejects. successful sections render, only the failed one is empty (with a specific message), isPageEmpty stays truthful!
| const pantryData = await ApiClient.getPantry(pantryId); | ||
| const pantryId = await ApiClient.getCurrentUserPantryId(); | ||
| const [pantryData, pantryFoodRequests, pantryOrders] = | ||
| await Promise.all([ |
There was a problem hiding this comment.
keep per-section catches, wrap in awaited Promise.all for the loading flag (see admin comment). the leading getXId/getMe call is a prerequisite, so let that one throw, then Promise.all the rest
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
please make the changes to pantry name & resolve merge conflicts before merging into main!
thanks for this justin! this is looking quite awesome. nice pr!
| }, [setAlertMessage]); | ||
|
|
||
| if (!pantry) return null; | ||
| if (loading || !pantry) return null; |
There was a problem hiding this comment.
can you remove the !pantry?
if only fetchPantry fails, pantry stays null (hitting !pantry) and the whole page renders null rather than the other sections
| @@ -77,51 +101,85 @@ const PantryDashboard: React.FC = () => { | |||
| Welcome, {pantry.pantryName} | |||
There was a problem hiding this comment.
to support a case of !pantry:
make this pantry?.pantryName
| type={DashboardCardType.FOOD_REQUEST} | ||
| title={`Request #${fr.requestId}`} | ||
| date={fr.requestedAt} | ||
| subtitle={pantry.pantryName} |
There was a problem hiding this comment.
and this to pantry?.pantryName
3 participants
ℹ️ Issue
Closes #206
📝 Description
I implemented SectionEmptyState and PageEmptyState components to update the ui when there isn't any data to display to users.
✔️ Verification
Tested empty state by setting conditionals that check if a section is empty to true
Dashboard empty state
Section empty state
🏕️ (Optional) Future Work / Notes
I had trouble testing Pantry dashboard specifically if(!pantry) { return null } in apps/frontend/src/containers/pantryDashboard.tsx made it so that the empty state message doesnt show so I added an error message for when there isn't data to display which seems to fix the empty state message not showing but I don't think that error message is needed.