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

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7301fbb
feat(Datagrid): change slug to aiLabel
sangeethababu9223 Oct 3, 2024
ea066cb
Merge remote-tracking branch 'upstream/main' into feat/datagrid-slug-…
sangeethababu9223 Oct 3, 2024
ed5d9f8
feat(Datagrid): refactor slug to aiLabel
sangeethababu9223 Oct 3, 2024
380ccf3
fix(Datagrid): fix example issue
sangeethababu9223 Oct 3, 2024
eddf010
fix(datagrid): folder name case issue update
sangeethababu9223 Oct 3, 2024
4db828d
fix(datagrid): ci-check issue
sangeethababu9223 Oct 3, 2024
48c7ef0
fix(Datagrid): ci-check fix
sangeethababu9223 Oct 3, 2024
c7ce3f9
feat(Datagrid): slug to aiLabel style and class updates
sangeethababu9223 Oct 3, 2024
38615c6
Merge branch 'main' into feat/datagrid-slug-to-ailabel
sangeethababu9223 Oct 3, 2024
cb40ce2
feat(Datagrid): update type for slug and aiLabel
sangeethababu9223 Oct 4, 2024
f233ccd
Merge branch 'main' into feat/datagrid-slug-to-ailabel
davidmenendez Oct 4, 2024
7645bc0
Merge branch 'main' into feat/datagrid-slug-to-ailabel
sangeethababu9223 Oct 5, 2024
d850573
Merge remote-tracking branch 'upstream/main' into feat/datagrid-slug-…
sangeethababu9223 Oct 8, 2024
054fff6
feat(Datagrid): slug to ai pr comments
sangeethababu9223 Oct 8, 2024
dc3980a
Merge branch 'feat/datagrid-slug-to-ailabel' of https://github.com/sa…
sangeethababu9223 Oct 8, 2024
0e09fe8
fix(Datagrid): ci-check issue
sangeethababu9223 Oct 8, 2024
828f533
feat(datagrid): slug to ai PR comments
sangeethababu9223 Oct 8, 2024
07f18e0
feat(Datagrid): pr comments
sangeethababu9223 Oct 8, 2024
a3232f6
fix(Datagrid): ci-check issue test
sangeethababu9223 Oct 8, 2024
5197b11
fix(Datagrid): case issue fix rename
sangeethababu9223 Oct 8, 2024
4c14a2b
Merge branch 'main' into feat/datagrid-slug-to-ailabel
sangeethababu9223 Oct 11, 2024
38679b5
feat(Datagrid): aiLabel classes updated per core carbon
sangeethababu9223 Oct 11, 2024
1ca0cc5
Merge branch 'main' into feat/datagrid-slug-to-ailabel
sangeethababu9223 Oct 21, 2024
da518f4
Merge branch 'main' into feat/datagrid-slug-to-ailabel
sangeethababu9223 Oct 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.

.#{$block-class} th.#{$block-class}__with-slug {
/* stylelint-disable-next-line carbon/theme-token-use */
box-shadow: inset 0 1px $ai-border-strong;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.


.#{$block-class}__defaultStringRenderer.#{$block-class}__defaultStringRenderer--aiLabel {
width: fit-content;
}

.#{$block-class}__expanded-row {
display: flex;
Expand Down Expand Up @@ -878,9 +959,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.

&.#{$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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.

[`${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.

})}
onMouseEnter={(event) => toggleParentHoverClass(event, 'enter')}
onMouseLeave={(event) => toggleParentHoverClass(event)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

import { useInitialColumnSort } from '../useInitialColumnSort';
import {
DataGridHeader,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
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.

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

},
headerProps.className
)}
Expand All @@ -252,7 +257,9 @@ const HeaderRow = (
{...getAccessibilityProps(header)}
>
{header.render('Header')}
{renderSlug(header.slug)}
{header.aiLabel
? renderAILabel(header.aiLabel)
: renderAILabel(header.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
{header.aiLabel
? renderAILabel(header.aiLabel)
: renderAILabel(header.slug)}
{renderAILabel(header.aiLabel || 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.

Thanks @devadula-nandan.
This is updated.

{resizerProps && !header.isAction && (
<ResizeHeader
{...{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
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

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.

import { DataGridState } from '../types';

const blockClass = `${pkg.prefix}--datagrid`;
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Copy link
Contributor

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}__slug--row`]: 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.

[`${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.

});

const withActionsColumn = headers
Expand All @@ -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`} />
Expand Down Expand Up @@ -213,6 +225,10 @@ const DatagridRow = (datagridState: DataGridState) => {
associatedHeader &&
associatedHeader.length &&
isValidElement(associatedHeader[0]?.slug),
Comment on lines 225 to 227
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.

},
columnClassname
)}
Expand Down
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.

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.

Loading
Loading