-
Notifications
You must be signed in to change notification settings - Fork 0
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(ui): migrate ComboBox component to typescript #589
Conversation
|
You delete everything where is the changes? :) I mean if it is a draft or not finished please close the PR |
@barsukov @guoda-puidokaite can you take a look? |
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.
LGTM
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.
Few comments from my side
packages/ui-components/src/components/ComboBox/ComboBox.stories.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBox/ComboBox.stories.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBox/ComboBox.stories.tsx
Outdated
Show resolved
Hide resolved
🦋 Changeset detectedLatest commit: c76db3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
A few things from my side
packages/ui-components/src/components/ComboBox/ComboBox.component.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBox/ComboBox.component.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBoxOption/ComboBoxOption.component.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBox/ComboBox.component.tsx
Outdated
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.
(Blocking)
Prettier issue in pipeline
packages/ui-components/src/components/ComboBox/ComboBox.component.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBoxOption/ComboBoxOption.component.tsx
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBoxOption/ComboBoxOption.component.tsx
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBoxOption/ComboBoxOption.component.tsx
Show resolved
Hide resolved
packages/ui-components/src/components/ComboBox/ComboBox.component.tsx
Outdated
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.
Please also change index file, to return deprecated combobox cause it is still use a portal provider until we done with all related portal provider messages @guoda-puidokaite could you also check the warnings in console if it would be introduce here?
…/cloudoperators/juno into gjaskiewicz/refactor-combobox-ts
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.
(Blocking)
Discrepancies in Storybook (currently live on left, PR on right):
ComboBoxOption
:children
missing inprops
ComboBox
: Breaking type change? The updated type forhelptext
,errortext
andsuccesstext
is more restrictive thanReactNode
. Could these be breaking changes?
ComboBox
: Are these function controls correct? Doesn't look right, but not sure. We didn't have any controls in the past. 🤷♀️
Before requesting another review, please double check that the old Storybook has no regressions. 🙏 It's difficult, as there's so many props in this one! But we can do it with few eyes on it. 👀
@gjaskiewicz Kindly address the issues identified by Guoda. It seems there are some regressions that need attention. |
…/cloudoperators/juno into gjaskiewicz/refactor-combobox-ts
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 great, Thank You for the fixes! 🚀💪
Love that you added some descriptions to the docs for ComboBoxOptions
, just made an observation on how we might be able to improve them. Change not required. 🙏
@andypf @barsukov, since this PR has so many comments, would be lovely if both of you could review too.
packages/ui-components/src/components/ComboBoxOption/ComboBoxOption.component.tsx
Outdated
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.
LGTM
Summary
Migrate combobox to TypeScript
Changes Made
Related Issues
Testing Instructions
npm i
npm run storybook
Checklist