-
Notifications
You must be signed in to change notification settings - Fork 19
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(EmptyState): support icons passed directly to EmptyStateHeader #612
Conversation
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.
Lookin 🔥!
I just have one type question and a small refactor request.
}; | ||
|
||
const emptyStateIconComponent = iconPropIsEmptyStateIconComponent() | ||
? (iconPropValue as JSXElement) |
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.
Is this type assertion definitely valid?
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.
Yes, it is quite indirect, but the iconPropIsEmptyStateIconComponent
function also checks that iconElementIdentifier
is not undefined, which is true when iconPropValue?.type === "JSXElement"
.
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 if there is no EmptyStateIcon
import? Would that cause emptyStateIconImport
to be undefined
and iconPropIsEmptyStateIconComponent
to incorrectly return true (since undefined === undefined
is true)?
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.
Oh, good catch, that could cause some trouble, thanks for finding that!
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.
That's what I'm here for! 🙂
? context | ||
.getSourceCode() | ||
.getText(emptyStateIconComponentIconAttribute) | ||
: iconPropIsIconElement() | ||
? `icon={${iconElementIdentifier!.name}}` |
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.
Is there a way to refactor this logic to avoid the nested ternary?
fe699ea
to
c227531
Compare
Uses helpers from #602 which should be merged first before this PR.
Closes #595