-
Notifications
You must be signed in to change notification settings - Fork 139
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(Datagrid): slug to aiLabel renaming #6151
Changes from 12 commits
7301fbb
ea066cb
ed5d9f8
380ccf3
eddf010
4db828d
48c7ef0
c7ce3f9
38615c6
cb40ce2
f233ccd
7645bc0
d850573
054fff6
dc3980a
0e09fe8
828f533
07f18e0
a3232f6
5197b11
4c14a2b
38679b5
1ca0cc5
da518f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,6 +156,7 @@ | |
background-color: $background; | ||
} | ||
|
||
/* This section to be removed after support for slug dropped - start */ | ||
.#{$block-class} th.#{$block-class}__with-slug { | ||
/* stylelint-disable-next-line carbon/theme-token-use */ | ||
box-shadow: inset 0 1px $ai-border-strong; | ||
|
@@ -228,6 +229,80 @@ | |
.#{c4p-settings.$carbon-prefix}--slug { | ||
margin-left: $spacing-03; | ||
} | ||
/* This section to be removed after support for slug dropped - end */ | ||
|
||
.#{$block-class} th.#{$block-class}__with-aiLabel { | ||
/* stylelint-disable-next-line carbon/theme-token-use */ | ||
box-shadow: inset 0 1px $ai-border-strong; | ||
} | ||
|
||
.#{$block-class} th.#{$block-class}__with-aiLabel, | ||
.#{$block-class} td.#{$block-class}__aiLabel--cell { | ||
@include ai-table-gradient(); | ||
} | ||
|
||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row, | ||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row | ||
+ .#{$block-class}__expanded-row { | ||
@include ai-table-gradient(); | ||
|
||
background-attachment: fixed; | ||
} | ||
|
||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row { | ||
/* stylelint-disable-next-line carbon/theme-token-use */ | ||
box-shadow: inset 1px 0 $ai-border-strong; | ||
} | ||
|
||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row:hover, | ||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row.#{c4p-settings.$carbon-prefix}--data-table--selected:hover, | ||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row.#{$block-class}__carbon-row-expanded:hover | ||
+ .#{$block-class}__expanded-row, | ||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__expandable-row--hover.#{$block-class}__aiLabel--row { | ||
@include ai-table-gradient('hover'); | ||
} | ||
|
||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__expandable-row--hover.#{$block-class}__aiLabel--row | ||
td { | ||
background-color: transparent; | ||
} | ||
|
||
.#{$block-class} | ||
.#{c4p-settings.$carbon-prefix}--data-table | ||
tbody | ||
tr.#{$block-class}__aiLabel--row.#{c4p-settings.$carbon-prefix}--data-table--selected { | ||
@include ai-table-gradient('selected'); | ||
} | ||
|
||
.#{$block-class} | ||
th.#{$block-class}__with-aiLabel | ||
.#{c4p-settings.$carbon-prefix}--slug { | ||
margin-left: $spacing-03; | ||
} | ||
|
||
.#{$block-class}__grid-container { | ||
display: block; | ||
|
@@ -315,9 +390,15 @@ | |
white-space: initial; | ||
} | ||
|
||
/* This section to be removed after support for slug dropped - start */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here too, this could be removed in this pr itself, and add corresponding aiLabel classname conditionally on presence of aiLabel || slug prop? can refer alex pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some adapters are using slug classes for customising the style for the values passed as slug. Hence completely removing the classes could break such code. To be on the safer side it would be better to keep the slug classes until we completely remove the support for slug. |
||
.#{$block-class}__defaultStringRenderer.#{$block-class}__defaultStringRenderer--slug { | ||
width: fit-content; | ||
} | ||
/* This section to be removed after support for slug dropped - end */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here too, this could be removed in this pr itself, and add corresponding aiLabel classname conditionally on presence of aiLabel || slug prop? can refer alex pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some adapters are using slug classes for customising the style for the values passed as slug. Hence completely removing the classes could break such code. To be on the safer side it would be better to keep the slug classes until we completely remove the support for slug. |
||
|
||
.#{$block-class}__defaultStringRenderer.#{$block-class}__defaultStringRenderer--aiLabel { | ||
width: fit-content; | ||
} | ||
|
||
.#{$block-class}__expanded-row { | ||
display: flex; | ||
|
@@ -889,9 +970,16 @@ | |
.#{$block-class} .#{$block-class}__table-row-ai-enabled { | ||
display: flex; | ||
align-items: center; | ||
|
||
/* This section to be removed after support for slug dropped - start */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here too, this could be removed in this pr itself, and add corresponding aiLabel classname conditionally on presence of aiLabel || slug prop? can refer alex pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some adapters are using slug classes for customising the style for the values passed as slug. Hence completely removing the classes could break such code. To be on the safer side it would be better to keep the slug classes until we completely remove the support for slug. |
||
&.#{$block-class}__slug--expanded { | ||
border: none; | ||
} | ||
/* This section to be removed after support for slug dropped - end */ | ||
|
||
&.#{$block-class}__aiLabel--expanded { | ||
border: none; | ||
} | ||
} | ||
|
||
.#{$block-class} .#{$block-class}__table-row-ai-spacer, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -37,6 +37,9 @@ const DatagridExpandedRow = | |||||
<tr | ||||||
className={cx(`${blockClass}__expanded-row`, { | ||||||
[`${blockClass}__slug--row`]: isValidElement(row?.original?.slug), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be removed as we apply aiLabel className on both the props
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some adapters are using slug classes for customising the style for the values passed as slug. Hence completely removing the classes could break such code. To be on the safer side it would be better to keep the slug classes until we completely remove the support for slug. |
||||||
[`${blockClass}__aiLabel--row`]: isValidElement( | ||||||
row?.original?.aiLabel | ||||||
), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||
})} | ||||||
onMouseEnter={(event) => toggleParentHoverClass(event, 'enter')} | ||||||
onMouseLeave={(event) => toggleParentHoverClass(event)} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,7 +22,7 @@ import { | |||||||||
handleColumnResizingEvent, | ||||||||||
} from './addons/stateReducer'; | ||||||||||
import { getNodeTextContent } from '../../../global/js/utils/getNodeTextContent'; | ||||||||||
import { DatagridSlug } from './addons/Slug/DatagridSlug'; | ||||||||||
import { DatagridAILabel } from './addons/AILAbel/DatagridLabel'; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can fix the naming once we get a clarity on this |
||||||||||
import { useInitialColumnSort } from '../useInitialColumnSort'; | ||||||||||
import { | ||||||||||
DataGridHeader, | ||||||||||
|
@@ -182,14 +182,17 @@ const HeaderRow = ( | |||||||||
const { className: headerGroupClassName, ...headerGroupProps } = | ||||||||||
headerGroup.getHeaderGroupProps({ role: undefined }); | ||||||||||
|
||||||||||
const renderSlug = (slug) => { | ||||||||||
const renderAILabel = (aiLabel) => { | ||||||||||
if (isTableSortable) { | ||||||||||
return; | ||||||||||
} | ||||||||||
return <DatagridSlug slug={slug} />; | ||||||||||
return <DatagridAILabel aiLabel={aiLabel} />; | ||||||||||
}; | ||||||||||
|
||||||||||
const foundAIRow = rows.some((r) => isValidElement(r?.original?.slug)); | ||||||||||
const foundAIRow = rows.some( | ||||||||||
(r) => | ||||||||||
isValidElement(r?.original?.aiLabel) || isValidElement(r?.original?.slug) | ||||||||||
); | ||||||||||
const { key, ...rowProps } = headerGroupProps; | ||||||||||
const withActionsColumn = headers | ||||||||||
? !!headers.filter((header) => header.isAction).length | ||||||||||
|
@@ -244,6 +247,8 @@ const HeaderRow = ( | |||||||||
[`${blockClass}__header-actions-column`]: header?.isAction, | ||||||||||
[`${blockClass}__with-slug`]: | ||||||||||
header.slug && React.isValidElement(header?.slug), | ||||||||||
Comment on lines
251
to
252
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be removed and as we are going to apply aiLable corresponding classname on presence of slug prop or aiLabel prop
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||||||
[`${blockClass}__with-aiLabel`]: | ||||||||||
header.aiLabel && React.isValidElement(header?.aiLabel), | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||||||
}, | ||||||||||
headerProps.className | ||||||||||
)} | ||||||||||
|
@@ -252,7 +257,9 @@ const HeaderRow = ( | |||||||||
{...getAccessibilityProps(header)} | ||||||||||
> | ||||||||||
{header.render('Header')} | ||||||||||
{renderSlug(header.slug)} | ||||||||||
{header.aiLabel | ||||||||||
? renderAILabel(header.aiLabel) | ||||||||||
: renderAILabel(header.slug)} | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @devadula-nandan. |
||||||||||
{resizerProps && !header.isAction && ( | ||||||||||
<ResizeHeader | ||||||||||
{...{ | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,7 +11,7 @@ import { px } from '@carbon/layout'; | |||||
import { selectionColumnId } from '../common-column-ids'; | ||||||
import cx from 'classnames'; | ||||||
import { pkg, carbon } from '../../../settings'; | ||||||
import { DatagridSlug } from './addons/Slug/DatagridSlug'; | ||||||
import { DatagridAILabel } from './addons/AILAbel/DatagridLabel'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can fix the naming once we get a clarity on this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @devadula-nandan. |
||||||
import { DataGridState } from '../types'; | ||||||
|
||||||
const blockClass = `${pkg.prefix}--datagrid`; | ||||||
|
@@ -132,7 +132,10 @@ const DatagridRow = (datagridState: DataGridState) => { | |||||
}; | ||||||
|
||||||
const { className, ...rowProps } = row.getRowProps({ role: undefined }); | ||||||
const foundAIRow = rows.some((r) => isValidElement(r?.original?.slug)); | ||||||
const foundAIRow = rows.some( | ||||||
(r) => | ||||||
isValidElement(r?.original?.aiLabel) || isValidElement(r?.original?.slug) | ||||||
); | ||||||
|
||||||
const rowClassNames = cx(`${blockClass}__carbon-row`, { | ||||||
[`${blockClass}__carbon-row-expanded`]: row.isExpanded, | ||||||
|
@@ -141,6 +144,7 @@ const DatagridRow = (datagridState: DataGridState) => { | |||||
getAsyncSubRows && row.depth > 0, | ||||||
[`${carbon.prefix}--data-table--selected`]: row.isSelected, | ||||||
[`${blockClass}__slug--row`]: isValidElement(row?.original?.slug), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be removed and as we are going to apply aiLable corresponding classname on presence of slug prop or aiLabel prop
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||
[`${blockClass}__aiLabel--row`]: isValidElement(row?.original?.aiLabel), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||
}); | ||||||
|
||||||
const withActionsColumn = headers | ||||||
|
@@ -162,13 +166,21 @@ const DatagridRow = (datagridState: DataGridState) => { | |||||
{...setAdditionalRowProps()} | ||||||
> | ||||||
{foundAIRow ? ( | ||||||
row?.original?.slug ? ( | ||||||
row?.original?.aiLabel ? ( | ||||||
<td | ||||||
className={cx(`${blockClass}__table-row-ai-enabled`, { | ||||||
[`${blockClass}__aiLabel--expanded`]: row.isExpanded, | ||||||
})} | ||||||
> | ||||||
<DatagridAILabel aiLabel={row?.original?.aiLabel} /> | ||||||
</td> | ||||||
) : row?.original?.slug ? ( | ||||||
<td | ||||||
className={cx(`${blockClass}__table-row-ai-enabled`, { | ||||||
[`${blockClass}__slug--expanded`]: row.isExpanded, | ||||||
})} | ||||||
> | ||||||
<DatagridSlug slug={row?.original?.slug} /> | ||||||
<DatagridAILabel aiLabel={row?.original?.slug} /> | ||||||
</td> | ||||||
) : ( | ||||||
<td className={`${blockClass}__table-row-ai-spacer`} /> | ||||||
|
@@ -213,6 +225,10 @@ const DatagridRow = (datagridState: DataGridState) => { | |||||
associatedHeader && | ||||||
associatedHeader.length && | ||||||
isValidElement(associatedHeader[0]?.slug), | ||||||
Comment on lines
225
to
227
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these and above line can be removed, as we apply corresponding aiLabel classname conditionally on presence of aiLable or slug prop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||
[`${blockClass}__aiLabel--cell`]: | ||||||
associatedHeader && | ||||||
associatedHeader.length && | ||||||
isValidElement(associatedHeader[0]?.aiLabel), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since some adapters are already using the existing slug classes, completely changing the classnames would break their code. Hence a separate class is added for AiLabel. Once we stop the support for slug completely, we could just remove the slug classes. For now it would be safer to keep separate classes. |
||||||
}, | ||||||
columnClassname | ||||||
)} | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the file name be DatagridAiLabel.tsx? and the folder name as AiLabel? as we are following pascal case? @AlexanderMelox any idea on what to use here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @devadula-nandan. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/** | ||
* Copyright IBM Corp. 2024, 2024 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React, { | ||
ForwardedRef, | ||
ReactNode, | ||
forwardRef, | ||
isValidElement, | ||
} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
interface DatagridAILabelProps { | ||
aiLabel?: ReactNode; | ||
} | ||
|
||
interface NormalizedAILabeProps { | ||
size?: string; | ||
ref?: ForwardedRef<HTMLDivElement>; | ||
} | ||
|
||
export const DatagridAILabel = forwardRef( | ||
({ aiLabel }: DatagridAILabelProps, ref: ForwardedRef<HTMLDivElement>) => { | ||
if (aiLabel && isValidElement(aiLabel)) { | ||
const normalizedAILabel = React.cloneElement(aiLabel, { | ||
size: 'mini', | ||
ref, | ||
} as NormalizedAILabeProps); | ||
return normalizedAILabel; | ||
} | ||
return null; | ||
} | ||
); | ||
|
||
DatagridAILabel.propTypes = { | ||
/** | ||
* Specify the AI AILabel to be displayed | ||
*/ | ||
aiLabel: PropTypes.node, | ||
}; |
This file was deleted.
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.
can we remove this section in this pr itself? and add corresponding aiLabel class names conditionally on presence of aiLabel || slug prop? can refer alex 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.
Some adapters are using slug classes for customising the style for the values passed as slug. Hence completely removing the classes could break such code. To be on the safer side it would be better to keep the slug classes until we completely remove the support for slug.
For now, I've left this as it is. I tried to refactor this section by adding aiLabel classes along with the slug classes. But this feels too bulky and not so readable. Since we are going to remove the slug classes in future anyway, it felt this would make more sense.