-
Notifications
You must be signed in to change notification settings - Fork 94
Migration carbon v11 #1522
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
Migration carbon v11 #1522
Conversation
|
@asirvadAbrahamVarghese is attempting to deploy a commit to the data-driven-forms Team on Vercel. A member of the Team first needs to authorize it. |
|
@Hyperkid123 This was implemented prior to the cc: @Fryguy |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1522 +/- ##
==========================================
+ Coverage 94.27% 94.28% +0.01%
==========================================
Files 185 185
Lines 3389 3397 +8
Branches 1462 1461 -1
==========================================
+ Hits 3195 3203 +8
Misses 194 194 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "webpack-dev-server": "^4.7.3" | ||
| }, | ||
| "resolutions": { | ||
| "react-is": "^19.0.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.
react-is has moved from being a hard dependency to instead being a peer dependency. This change was done to facilitate React 19 support, so we will need this resolution for Tabs component to get rendered.
| <Button | ||
| disabled={fieldArrayProps.fields.length >= maxItems} | ||
| renderIcon={AddAlt32} | ||
| renderIcon={(props) => <AddAlt size={32} {...props} />} |
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.
For button's renderIcon, passing the props to the function component is needed for the proper styles to get applied to the icon(missing classes on the SVG element)
See: carbon-design-system/carbon#11168
| {selectedValues.includes(value) && <CheckmarkFilled16 />} | ||
| </Column> | ||
| </Row> | ||
| <Grid condensed {...GridProps}> |
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.
The Grid system has changed. The Row component no longer exists - it's been removed. In v11, we should use Grid and Column directly without Row.
See: https://github.com/carbon-design-system/carbon/blob/main/docs/migration/v11.md#grid
|
|
||
| const SingleCheckboxInCommon = ({ label, isDisabled, id, meta, option: { value, name, ...rest }, onChange, ...props }) => ( | ||
| <CarbonCheckbox id={id} labelText={label} disabled={isDisabled} {...props} {...rest} onChange={(_value, _name, event) => onChange(event)} /> | ||
| <CarbonCheckbox id={id} labelText={label} disabled={isDisabled} {...props} {...rest} onChange={(event, data) => onChange(event, data)} /> |
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.
In Carbon v11, the Checkbox onChange callback receives:
- event - The change event
- an object with { checked, id } - The new checked state and the checkbox ID
See: https://github.com/carbon-design-system/carbon/blob/main/docs/migration/v11.md#checkbox
| return ( | ||
| <div {...WrapperProps}> | ||
| <Toggle {...inputRest} toggled={checked} key={input.name} id={input.name} labelA={offText} labelB={onText} {...rest} /> | ||
| <Toggle {...inputRest} toggled={checked} onToggle={onChange} key={name} id={name} labelA={offText} labelB={onText} {...rest} /> |
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.
In Carbon v11, the Toggle component structure has changed - The underlying element has been changed from a checkbox (<input type="checkbox">) to a switch button (<button role="switch">).
We can use either onClick or onToggle, going with onToggle because it provides the checked state
See: https://github.com/carbon-design-system/carbon/blob/main/docs/migration/v11.md#toggle
| const Component = input.type === 'number' ? NumberInput : TextInput; | ||
|
|
||
| const setValue = (e, input) => (input.type === 'number' ? e.imaginaryTarget.value : e.target.value); | ||
| const setValue = (e, state, input) => (input.type === 'number' ? `${state.value}` : e.target.value); |
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.
In carbon-11 for NumberInput, imaginaryTarget is no longer available on the onChange event. The signature for onChange is onChange(event, {value, direction})
See: https://github.com/carbon-design-system/carbon/blob/main/docs/migration/v11.md#numberinput
| <div {...WrapperProps}> | ||
| <CarbonDatePicker {...input} datePickerType={datePickerType} {...DatePickerProps}> | ||
| <DatePickerInput id={input.name} invalid={Boolean(invalid)} invalidText={invalid ? invalid : ''} {...rest} /> | ||
| <CarbonDatePicker {...input} datePickerType={datePickerType} invalid={Boolean(invalid)} {...DatePickerProps}> |
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.
invalid prop on DatePickerInput has no impact now. So invalid should be passed to DatePicker and invalidText should be passed to DatePickerInput(as it is now) - this is not referenced in the migration docs, after discussing with the Carbon team, I have opened an issue: carbon-design-system/carbon#21035
|
Hello @asirvadAbrahamVarghese I'll do a review shortly. Just FYI, we are currently migrating to a new release process and will be doing some additional housekeeping soon. There may be a few conflicts appearing in this PR shortly. |
Hyperkid123
left a 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.
This is great!
|
Thank you foe the contribution @asirvadAbrahamVarghese. All works as expected. |
|
🎉 This PR is included in version 4.1.2 🎉 The release is available on |
|
Thanks @Hyperkid123 and @rvsia ❤️ |
Fixes #1300
Description
PR with carbon-11 migration.
References:
https://carbondesignsystem.com/migrating/guide/develop/
https://github.com/carbon-design-system/carbon/blob/main/docs/migration/v11.md
Schema (if applicable)
Checklist: (please see documentation page for more information)
Yarn buildpassesYarn lintpassesYarn testpassesfix|feat({scope}): {description}fix(pf3): wizard correctly handles next buttonFix button on documenation example page