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(AILabel): Rename Slug to AILabel #16803

Merged

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented Jun 17, 2024

Closes #16762
Closes #16618
Closes #16756
Closes #16755

Renames the React component Slug, SlugContent and SlugActions to AILabel, AILabelContent, and AILabelActions

Changelog

New

  • AILabel, AILabelContent and AILabelActions components. Should be identical to the existing Slug named components
  • AiSkeleton components renamed to AISkeleton

Changed

  • Deprecated aiTextLabel and SlugLabel and replaced them with textLabel and aria-label
  • Reorogranized all Slug stories to live under their individual components or under the AILabel section
  • All Slug references renamed to AILabel throughout .mdx files and documentation

Removed

  • Outdated documentation / story files

Testing / Reviewing

No style classes or prop names for other components were changed. All changes should be scoped to move from src/components/Slug to /src/components/AILabel and update associated tests/file imports. Same with the AiSkeleton to AISkeleton changes.

unstable__Slug and co. is still being exported and can still be used.

Copy link

netlify bot commented Jun 17, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 65f6395
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66a8f95a262d15000833c5d8
😎 Deploy Preview https://deploy-preview-16803--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 Jun 17, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 65f6395
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66a8f95af1fbda00084a3236
😎 Deploy Preview https://deploy-preview-16803--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.

@tw15egan tw15egan marked this pull request as ready for review June 27, 2024 15:17
@tw15egan tw15egan requested review from a team as code owners June 27, 2024 15:17
@@ -111,7 +111,7 @@ const DismissibleTag = <T extends React.ElementType>({
};

let normalizedSlug;
Copy link
Member

@alisonjoseph alisonjoseph Jun 28, 2024

Choose a reason for hiding this comment

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

quick question before I fully review, should we rename these also? (slug, normalizedSlug, slugRef etc)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn’t planning on changing the prop names for each component, so I didn’t touch the internal variable names either 🤷‍♂️

Copy link
Member

@tay1orjones tay1orjones Jul 3, 2024

Choose a reason for hiding this comment

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

Maybe we could roll those changes up into a sub item of #15972? Since ideally it would need to ship with a codemod

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me 👍

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Couple minor things, do we want to move the AI Skeleton component into its own section. Right now the overview seems like it might apply to all Skeleton components instead of just AI.
Screenshot 2024-07-09 at 12 25 35 PM

Datatables aren't showing full width for some reason on the overview page.
Screenshot 2024-07-09 at 12 27 46 PM

Otherwise LGTM!

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

I noticed the focus state on the mini and 2sx sizes are incorrect. They should be 2px thick (not 1) to have a visual distinction from the enabled state.

@aagonzales
Copy link
Member

Also as an aside, @mbgower has pointed out that the mini and 2xs size slugs inside the inputs will have a violation with a new WCAG rule come October. They will need to have 24x24 target size when placed inside another interactive element like an input. Not sure if we need to figure out that problem before making it stable.

@mbgower
Copy link

mbgower commented Jul 12, 2024

Further to @aagonzales's comment, there is a provision for under-sized targets, but they are only allowed if they have sufficient space around them such that when assessing with a 24px diameter circle no other target is intersected.
image

I realize this is not specific to the "rename" task, but since she raised it, I thought I'd add a bit more context.

@mbgower
Copy link

mbgower commented Jul 12, 2024

I’ve been reviewing the documentation for the “AI label” component, and I would like to advocate rebranding this as “AI indicator” or some other alternative (I have a number of other options). The problem with calling it a label is that it isn’t a label, it’s a button. I don’t think it has to be called a button (hence my suggestion of ‘indicator’), but a label is an intrinsic part of almost every UI component, and there are knock-on effects from using it as part of a component name.
In example, there are points where the documentation talks about the AI label but it is specifically talking about the AI label text (i.e., “AI”) not the component. It confused me for some time until I understood what was being said. You’re also at some point going to probably come up with an AI icon, which people will use to label buttons (or whatever) for different functions. At that point, discussions around the AI label are going to get more confusing (e.g., “I’m talking about the AI label component not the AI icon used as a label here”).
The AI label on the AI label is also at the uncomfortable edge of symbol versus text from a user perception POV (other examples are buttons labelled with a question mark, an ellipsis, the small letter i, the italicized capital letter i, the capital B, etc), so to the degree its possible to avoid further potential confusion, it’s good.

@tw15egan
Copy link
Collaborator Author

@alisonjoseph Moved all AISkeleton components to its own folder inside Skeleton, and fixed the issue with DataTable mdx files

@aagonzales fixed the focus states 👍

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM as long as the name issue is resolved.

@aagonzales
Copy link
Member

@tw15egan @alisonjoseph @tay1orjones Do you think we should also go ahead and update the click target size of the 2xs and mini to be 24px?

image

@mbgower
Copy link

mbgower commented Jul 17, 2024

As mentioned on a call, although it seems natural to extend the click area evenly beyond the visible target, a case can be made that when used with these inputs, it is as beneficial to the user if the extended target area is offset to the right of the visible area to eliminate that sliver of target for the input that wraps around the AI slug.

If the size of the + and - hit areas were replicated, this would be the result.
Screenshot 2024-07-17 at 6 37 25 AM

This breaks down a bit when considering what to do with say the target in a column header, where the offset would end up being a bit to the left. Such a strategy would need to be on an input-by-input basis.
So it may be easier just to evenly extend the hit area out to 24x24, but I just thought I'd flag the consideration.
Screenshot 2024-07-17 at 8 19 30 AM

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

The inline with text AI labels are coming in weird.
image

@tw15egan
Copy link
Collaborator Author

@aagonzales, Whoops, I just pushed up a fix for that! Also increased the click target to 24px for all mini and 2xs variants

@tw15egan
Copy link
Collaborator Author

@tay1orjones @alisonjoseph I also added a new ai-label entry point in @carbon/styles that just forwards along slug so the imports match when using the new AILabel name

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Those changes look good to me now!

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Looks great! I added the prop rename task as a subitem of #15972

@tay1orjones tay1orjones enabled auto-merge July 30, 2024 14:31
@tay1orjones tay1orjones added this pull request to the merge queue Jul 30, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 30, 2024
@tay1orjones tay1orjones added this pull request to the merge queue Aug 1, 2024
Merged via the queue into carbon-design-system:main with commit 753e3a2 Aug 1, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants