-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Search): noResultsMessage prop can be a node #1683
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1683 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 141 141
Lines 2410 2410
=======================================
Hits 2404 2404
Misses 6 6
Continue to review full report at Codecov.
|
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 one comment regarding propTypes and typings.
src/modules/Search/Search.js
Outdated
noResultsMessage: PropTypes.oneOfType([ | ||
PropTypes.node, | ||
PropTypes.object, | ||
]), |
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.
I believe this should just be PropTypes.node
only. PropTypes.node
includes any renderable value: string, number, element, or an array (or fragment) of any of these. However, objects are not renderable.
Let's also update the typings in Search.d.ts
to reflect the type change 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.
PropTypes.node includes any renderable value: string, number, element, or an array (or fragment) of any of these.
Didn't know about it. Thanks for useful information.
Made the necessary changes.
src/modules/Search/Search.d.ts
Outdated
@@ -30,7 +30,7 @@ export interface SearchProps { | |||
noResultsDescription?: string; |
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.
@b0gok Thanks for a PR. I think that we need make same changes for noResultsDescription
.
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.
@layershifter Should I add specific test for case when noResultsDescription isn't a string?
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 will be awesome.
Thank you for the PR and updates! |
Released in |
PR for this issue #1664