-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
style(Search): several cleanups #1150
Conversation
@@ -34,6 +34,7 @@ interface SearchProps extends ReactMouseEvents<HTMLInputElement>, ReactFocusEven | |||
/** Shorthand for Icon. */ | |||
icon?: any; | |||
|
|||
/** A search can show a loading indicator. */ |
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.
Added inline comment to prop.
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.
Thanks, we should also update the ts file comment as well.
'search', | ||
className, |
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.
className
is always last in our pattern.
{menuContent} | ||
</SearchResults> | ||
) | ||
return <SearchResults className={resultsClasses}>{menuContent}</SearchResults> |
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.
This expression is too simple to be multiline.
icon, | ||
}) | ||
) | ||
return Input.create(input, { |
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.
We don't really need parens there.
@@ -169,17 +169,19 @@ export default class Search extends Component { | |||
/** A search input can take up the width of its container. */ | |||
input: customPropTypes.itemShorthand, | |||
|
|||
size: PropTypes.oneOf(_meta.props.size), | |||
|
|||
/** A search can show a loading indicator. */ |
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.
Inline 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.
Let's also update the ts file.
loading: PropTypes.bool, | ||
|
||
/** A search can have different sizes. */ |
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.
^
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.
^ 😄
style(Search): propTypes cleanups, update comments
8f862ad
to
c3c6e65
Compare
I've updated ts-definition file and updated propTypes for #524. |
Current coverage is 95.88% (diff: 100%)
|
Thanks much! |
Released in |
I missed the moment when #1123 was merged, so I'll make several style cleanups in this PR.
This PR: