-
Notifications
You must be signed in to change notification settings - Fork 142
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
chore: tagoverflow typescript conversion #5715
chore: tagoverflow typescript conversion #5715
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Good work
Please see some minor comments
onOverflowTagChange = defaults.onOverflowTagChange, | ||
|
||
// Collect any other property values passed in. | ||
overflowType, |
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.
Hey, Looks like overflowType
is an optional prop and we were using a default value as 'default
' , and now you have removed passing default values.
Will that affect the behaviour?
Can we pass just like overflowType = 'default'
Please check other props as well where default values have been removed
@@ -292,7 +286,7 @@ const tagTypes = Object.keys(TYPES); | |||
*/ | |||
export const string_required_if_more_than_10_tags = isRequiredIf( | |||
PropTypes.string, | |||
({ items }) => items && items.length > allTagsModalSearchThreshold | |||
({ items }) => items && items?.length > allTagsModalSearchThreshold |
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.
Hey , do you think just this is enough, though both works?
({ items }) => items?.length > allTagsModalSearchThreshold
); | ||
}; | ||
|
||
const visibleItems = getOverflowPopoverItems(); | ||
const hasItems = visibleItems && visibleItems?.length > 0; |
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.
same comment as above
const hasItems = visibleItems?.length > 0;
@amal-k-joy please re-review when you have a chance. ty! |
id: string; | ||
label: string; | ||
onClose: () => void; | ||
tagType?: |
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.
If possible we should try to import this from @carbon/react
.
maxVisible?: number; | ||
measurementOffset?: number; | ||
multiline?: boolean; | ||
overflowAlign?: |
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.
If possible we should try to import this from @carbon/react.
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.
Found some types that we might be able to just pull in from @carbon/react
, but we can open a separate issue.
35ea4c4
Closes #5673