-
Notifications
You must be signed in to change notification settings - Fork 10
FIX:Improve the empty state experience to align with the new design. #796
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
Conversation
src/components/icons/index.ts
Outdated
| export * from './warning'; | ||
| export * from './warning-2'; | ||
| export * from './warning-3'; | ||
| export * from './empty-icon' |
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.
🟡 IMPORTANT: Missing semicolon
Problem:
Inconsistent with other export statements in the file which all end with semicolons.
Suggestion:
export * from './empty-icon';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.
Ajusted on: Link
| <BakoEmptyState.Title | ||
| fontSize="xs" | ||
| color="textPrimary" | ||
| color="gray.200" |
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.
🟡 IMPORTANT: Hardcoded color values
Problem:
Using hardcoded color values ('gray.200', 'gray.400') instead of semantic theme tokens may cause inconsistency with the design system.
Suggestion:
Use semantic color tokens from the theme:
color="textPrimary" // or appropriate semantic tokenThere 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 had to use it like this to keep it in line with the layout.
| w="full" | ||
| minH="230px" | ||
| display="flex" | ||
| {...rest} |
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.
🔵 SUGGESTION: Consider extracting gap value
Problem:
The gap value changed from 6 to 1, which is a significant visual change that might benefit from being configurable.
Suggestion:
Consider making gap configurable through props or use a theme token:
gap={gap ?? 1}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 gap is already in agreement
guimroque
left a comment
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.
Code Review - Summary
What was done
Updated the empty state components to align with new design specifications:
- Modified the main EmptyState component with new spacing, colors, and added an EmptyIcon
- Created a new EmptyIcon component using bako-ui's createIcon utility
- Updated the notifications empty state to use the new EmptyIcon instead of EmptyBoxOutline
Positive Points
- Clean implementation following the project's component structure
- Proper use of bako-ui components and patterns
- Good separation of concerns with the new icon component
- Consistent naming conventions (PascalCase for components)
- Proper export pattern in the icons index file
Issues Found
- Missing semicolon in export statement (style consistency)
- No TypeScript interface for EmptyStateProps extension
- Hardcoded color values instead of using theme tokens
Total comments: 3 (0 critical, 2 important, 1 suggestion)
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Description
Update the empty state to the new design, ensuring visual alignment with the updated layout and an improved user experience.
Summary
Screenshots
Empty State:

Notifications:

Checklist