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

chore: add decorator in dataTable #18114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

preetibansalui
Copy link
Contributor

Closes #18005

Add decorator in dataTable component

Changelog

Added a new component TableDecoratorRow
Added a new deprecated warning function for components

Changed

  1. marked slug as deprecated prop in all dataTable components
  2. marked TableSlugRow component as deprecated
  3. Added few decorators in storybook as ai-lable and decorator both.

Testing / Reviewing

  1. Go to Storybook > DataTable > AILabel story
  2. Check each story under that, All AI slug stories are now a mix of slug and decorator, so all should look fine.
  3. Added few examples of decorator Tooltip as well, that too should look fine.

Copy link

netlify bot commented Nov 19, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit e5ed412
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/673f1347ff46b50008269602
😎 Deploy Preview https://deploy-preview-18114--v11-carbon-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

netlify bot commented Nov 19, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit e5ed412
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/673f134737a460000820fa74
😎 Deploy Preview https://deploy-preview-18114--carbon-elements.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

netlify bot commented Nov 19, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e5ed412
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/673f13478ba1760008bfadd7
😎 Deploy Preview https://deploy-preview-18114--v11-carbon-react.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
Member

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

thanks for tackling the data table!!! this is looking really good! left some comments, but lmk if you have any questions!

margin-inline: auto convert.to-rem(8px);
}
.#{$prefix}--table-sort__header--slug .#{$prefix}--ai-label,
.#{$prefix}--table-sort__header--slug .#{$prefix}--slug {
.#{$prefix}--table-sort__header--slug .#{$prefix}--slug,
.#{$prefix}--table-sort__header--decorator .#{$prefix}--tooltip {
Copy link
Member

Choose a reason for hiding this comment

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

since we don't know what will be in the decorator prop there should be some inner wrapper to target something like:

Suggested change
.#{$prefix}--table-sort__header--decorator .#{$prefix}--tooltip {
.#{$prefix}--table-sort__header--decorator .#{$prefix}--table-sort__inner-header--decorator > * {

Comment on lines +20 to +24
/**
* @deprecated please use decorator instead.
* Provide a `Slug` component to be rendered inside the `TableSlugRow` component
*/
slug?: ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

do we need the deprecated slug if we are introducing a new component?

@@ -75,9 +75,15 @@ const TableExpandRow = React.forwardRef(

// We need to put the slug before the expansion arrow and all other table cells after the arrow.
let rowHasSlug;
Copy link
Member

Choose a reason for hiding this comment

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

since this is internal it might be nice to update to use AILabel instead to keep consistency and shouldn't break anything on the adopter side

Suggested change
let rowHasSlug;
let rowHasAILabel;

@@ -106,7 +115,7 @@ const TableExpandRow = React.forwardRef(

return (
<tr {...rest} ref={ref as never} className={className} data-parent-row>
{slug}
{decorator}
Copy link
Member

Choose a reason for hiding this comment

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

I can't comment on it, but after line 110: [${prefix}--data-table--slug-row]: rowHasSlug,
I think there should also be [${prefix}--data-table--ai-label-row]: rowHasAILabel,

so that we can eventually get rid of the the slug-row class in v12

Comment on lines 155 to 156
// Slug is always size `mini`
const slugRef = useRef<HTMLInputElement>(null);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Slug is always size `mini`
const slugRef = useRef<HTMLInputElement>(null);
// AILabel is always size `mini`
const AILabelRef = useRef<HTMLInputElement>(null);

}

const headerLabelClassNames = classNames({
[`${prefix}--table-header-label`]: true,
[`${prefix}--table-header-label--slug`]: slug,
[`${prefix}--table-header-label--slug`]: colHasSlug,
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure i am following why this needed to be updated to colHasSlug instead of slug if just decorator works below

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.

Decorator: DataTable
2 participants