Skip to content
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

Conversation

sangeethababu9223
Copy link
Member

@sangeethababu9223 sangeethababu9223 commented Oct 3, 2024

Closes #6097

Rename slug aiLabel.

What did you change?

  • Added new key aiLabel for column / row data
  • Updated Docs section with aiLabel
  • Deprecated Slug - Since slug is not a prop here but key value pair being passed as data, doc updated to inform the it will be supported for a limited time only in future.
  • Use AILabel component instead of Slug
  • Story renamed to AILabel from Slug, Since the same is done in core carbon
  • Updated the style with aiLabel classes as per the Core carbon standard

How did you test and verify your work?

Storybook

Copy link

netlify bot commented Oct 3, 2024

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit da518f4
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/6717a543f52fc60008305eb7
😎 Deploy Preview https://deploy-preview-6151--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -194,7 +194,8 @@ export interface DataGridHeader<T extends object = any>
UseSortByColumnProps<T> {
className(className: any, arg1: { [x: string]: any }): unknown;
isAction?: boolean;
slug?: any;
slug?: any; // To be removed once the support for slug is not available
aiLabel?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aiLabel?: any;
aiLabel?: ReactNode | boolean;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AlexanderMelox. I've updated the type for both slug and aiLabel to ReactNode. Since we would only be passing AILabel component as value for aiLabel, I've not added boolean here.

I've to look into a few more things for this issue, Once that is done I'll change the PR to ready for review

@sangeethababu9223 sangeethababu9223 marked this pull request as ready for review October 4, 2024 09:55
@sangeethababu9223 sangeethababu9223 requested a review from a team as a code owner October 4, 2024 09:55
@sangeethababu9223 sangeethababu9223 requested review from makafsal, devadula-nandan and matthewgallo and removed request for a team October 4, 2024 09:55
Copy link
Contributor

@devadula-nandan devadula-nandan Oct 7, 2024

Choose a reason for hiding this comment

The 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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @devadula-nandan.
This is updated.
I was facing some issues with git case sensitivity while renaming. But this is sorted now.

@@ -156,6 +156,7 @@
background-color: $background;
}

/* This section to be removed after support for slug dropped - start */
Copy link
Contributor

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.

Copy link
Member Author

@sangeethababu9223 sangeethababu9223 Oct 8, 2024

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.

@@ -315,9 +390,15 @@
white-space: initial;
}

/* This section to be removed after support for slug dropped - start */
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@sangeethababu9223 sangeethababu9223 Oct 8, 2024

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.

.#{$block-class}__defaultStringRenderer.#{$block-class}__defaultStringRenderer--slug {
width: fit-content;
}
/* This section to be removed after support for slug dropped - end */
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@sangeethababu9223 sangeethababu9223 Oct 8, 2024

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.
I've refactored the style in this section to avoid duplication of styles.

@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@sangeethababu9223 sangeethababu9223 Oct 8, 2024

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.
I've refactored the style in this section to avoid duplication of styles.

@@ -37,6 +37,9 @@ const DatagridExpandedRow =
<tr
className={cx(`${blockClass}__expanded-row`, {
[`${blockClass}__slug--row`]: isValidElement(row?.original?.slug),
Copy link
Contributor

Choose a reason for hiding this comment

The 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
[`${blockClass}__slug--row`]: isValidElement(row?.original?.slug),

Copy link
Member Author

@sangeethababu9223 sangeethababu9223 Oct 8, 2024

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.

@@ -37,6 +37,9 @@ const DatagridExpandedRow =
<tr
className={cx(`${blockClass}__expanded-row`, {
[`${blockClass}__slug--row`]: isValidElement(row?.original?.slug),
[`${blockClass}__aiLabel--row`]: isValidElement(
row?.original?.aiLabel
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
),
) || isValidElement(row?.original?.slug),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can fix the naming once we get a clarity on this

Comment on lines 248 to 249
[`${blockClass}__with-slug`]:
header.slug && React.isValidElement(header?.slug),
Copy link
Contributor

@devadula-nandan devadula-nandan Oct 7, 2024

Choose a reason for hiding this comment

The 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
[`${blockClass}__with-slug`]:
header.slug && React.isValidElement(header?.slug),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@@ -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),
[`${blockClass}__aiLabel--row`]: isValidElement(row?.original?.aiLabel),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[`${blockClass}__aiLabel--row`]: isValidElement(row?.original?.aiLabel),
[`${blockClass}__aiLabel--row`]: isValidElement(row?.original?.aiLabel || isValidElement(row?.original?.slug),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Comment on lines 225 to 227
associatedHeader &&
associatedHeader.length &&
isValidElement(associatedHeader[0]?.slug),
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isValidElement(associatedHeader[0]?.aiLabel),
(isValidElement(associatedHeader[0]?.aiLabel) || isValidElement(associatedHeader[0]?.slug),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@@ -38,39 +38,39 @@ export default {
blocks={[
{
description:
"A Carbon AI slug can be used within the Datagrid for both column headers and rows. To include a column header AI slug, include a `slug` property within your column definition and include the Slug component as it's own custom component",
"A Carbon AI Label can be used within the Datagrid for both column headers and rows. To include a column header AI Label, include a `aiLabel` property within your column definition and include the AILabel component as it's own custom component. <br/> The `slug` property has been deprecated and will be phased out soon. It will only be supported for a limited time in future. Please use `aiLabel` instead going forward.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"A Carbon AI Label can be used within the Datagrid for both column headers and rows. To include a column header AI Label, include a `aiLabel` property within your column definition and include the AILabel component as it's own custom component. <br/> The `slug` property has been deprecated and will be phased out soon. It will only be supported for a limited time in future. Please use `aiLabel` instead going forward.",
"A Carbon AI Label can be used within the Datagrid for both column headers and rows. To include a column header AI Label, include a `aiLabel` property within your column definition and include the AILabel component as it's own custom component. <br/> The `slug` property has been deprecated. Please use `aiLabel` property instead.",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @devadula-nandan.
This is updated.

Comment on lines 30 to 31
[`${blockClass}__defaultStringRenderer--slug`]:
slug && React.isValidElement(slug),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[`${blockClass}__defaultStringRenderer--slug`]:
slug && React.isValidElement(slug),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

return (
<div
className={cx(`${blockClass}__defaultStringRenderer`, {
[`${blockClass}__defaultStringRenderer--slug`]:
slug && React.isValidElement(slug),
[`${blockClass}__defaultStringRenderer--aiLabel`]:
aiLabel && React.isValidElement(aiLabel),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aiLabel && React.isValidElement(aiLabel),
(aiLabel && React.isValidElement(aiLabel)) || (slug && React.isValidElement(slug)),

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@@ -11,7 +11,7 @@ import { pkg, carbon } from '../../settings';
import { Button } from '@carbon/react';
import { ArrowUp, ArrowDown, ArrowsVertical } from '@carbon/react/icons';
import { SelectAll } from './Datagrid/DatagridSelectAll';
import { DatagridSlug } from './Datagrid/addons/Slug/DatagridSlug';
import { DatagridAILabel } from './Datagrid/addons/AILAbel/DatagridLabel';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can check with naming of folder / file

Comment on lines 159 to 163
{headerProp?.column?.aiLabel ? (
<DatagridAILabel aiLabel={headerProp?.column?.aiLabel} />
) : (
<DatagridAILabel aiLabel={headerProp?.column?.slug} />
)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{headerProp?.column?.aiLabel ? (
<DatagridAILabel aiLabel={headerProp?.column?.aiLabel} />
) : (
<DatagridAILabel aiLabel={headerProp?.column?.slug} />
)}
<DatagridAILabel aiLabel={headerProp?.column?.aiLabel || headerProp?.column?.slug} />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @devadula-nandan.
This is updated.

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for ibm-products-web-components ready!

Name Link
🔨 Latest commit da518f4
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/6717a5433af3230008aeca14
😎 Deploy Preview https://deploy-preview-6151--ibm-products-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 74.19355% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.72%. Comparing base (5c0f2ad) to head (da518f4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6151      +/-   ##
==========================================
- Coverage   78.72%   78.72%   -0.01%     
==========================================
  Files         395      395              
  Lines       12799    12811      +12     
  Branches     4215     4227      +12     
==========================================
+ Hits        10076    10085       +9     
- Misses       2723     2726       +3     
Components Coverage Δ
ibm-products ∅ <ø> (∅)
ibm-products-web-components ∅ <ø> (∅)

Copy link
Contributor

@devadula-nandan devadula-nandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sangeethababu9223 sangeethababu9223 added this pull request to the merge queue Oct 22, 2024
Merged via the queue into carbon-design-system:main with commit b0b3c1c Oct 22, 2024
28 checks passed
@sangeethababu9223 sangeethababu9223 deleted the feat/datagrid-slug-to-ailabel branch October 22, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datagrid: slug --> aiLabel renaming
5 participants