-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat(compass-saved-aggregations-queries): empty list items COMPASS-5539 #2847
Conversation
packages/compass-saved-aggregations-queries/src/components/aggregations-queries-list.tsx
Outdated
Show resolved
Hide resolved
const EmptyContent: React.FunctionComponent< | ||
EmptyContentProps & React.HTMLProps<HTMLDivElement> | ||
> = ({ icon, title, subTitle, callToAction }) => { | ||
const 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.
what's the advantage of this if
? isn't it better to just make the icon be a JSX.Element
and pass icon={<QuerySearchIcon>}
in EmptyContent
and icon={<SearchResultsIcon>}
in NoSearchResults
?
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.
actually that's a good idea.
Just wondering once we move this to compass-components
, it will be better to have these custom icons there and they can be reused in normal Icon
component as well.
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.
Just wondering once we move this to compass components, it will be better to have these custom icons there and they can be reused in normal Icon components as well.
Yeah, we could do that if those are reused for sure. There is no reason to couple this component with the actual graphic though. The EmptyContent
component can accomplish its role fully without knowing what graphic is being used, there is no advantage in giving it that knowledge.
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.
Awesome 👍
packages/compass-saved-aggregations-queries/src/components/aggregations-queries-list.tsx
Outdated
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Outdated
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Outdated
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Show resolved
Hide resolved
packages/compass-saved-aggregations-queries/src/components/empty-list-items.tsx
Show resolved
Hide resolved
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.
Looks good!
feat(compass-saved-aggregations-queries): empty list items COMPASS-5539
Screen.Recording.2022-02-28.at.18.11.25.mov
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes