Rename and deprecate Breadcrumb to Breadcrumbs #1448
Conversation
🦋 Changeset detectedLatest commit: a5c775a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
colebemis
changed the title
| * @deprecated Use the Breadcrumbs component instead (i.e. <Breadcrumb> → <Breadcrumbs>) | ||
| */ | ||
| export const Breadcrumb = Object.assign(Breadcrumbs, {Item: BreadcrumbsItem}) | ||
| export type BreadcrumbProps = ComponentProps<typeof BreadcrumbsBase> |
There was a problem hiding this comment.
Can we add deprecation warnings for these types as well?
There was a problem hiding this comment.
Sure thing! I followed the same approach as when Position subcomponents where deprecated (component had deprecation warning but not the props types). In any case makes sense to see the message in the IDE if you're importing the outdated types.
src/Breadcrumbs.tsx
Outdated
| function Breadcrumb({className, children, theme, ...rest}: React.PropsWithChildren<BreadcrumbProps>) { | ||
| const classes = classnames(className, 'Breadcrumb') | ||
| function Breadcrumbs({className, children, theme, ...rest}: React.PropsWithChildren<BreadcrumbsProps>) { | ||
| const classes = classnames(className, 'Breadcrumbs') |
There was a problem hiding this comment.
I wonder why we need to add this classname 🤔
There was a problem hiding this comment.
Git blame tells it has been there since the very beginning, don't think it's used for anything though.
There was a problem hiding this comment.
If there are no references it anywhere else in the codebase, let's go ahead and remove it
There was a problem hiding this comment.
There's a specific test case that validates a Breadcrum class exists, so it was done on purpose for what it seems. There're other primer react components with readable classes injected to them. But I don't think it's a convention as most components do not add them.
Either the removal or rename to Breadcrums will be breaking if someone relies on the class name for their tests (although it's not a good practice).
There was a problem hiding this comment.
Let's go ahead and remove it (and the test case) and add a changeset to indicate that it's a breaking change
Co-authored-by: Cole Bemis <colebemis@github.com>
Closes #1393
BreadcrumtoBreadcrumsandBreadcrumItemtoBreadcrumsItemwith their types.Screenshots
Merge checklist
Notes
Not sure if we should update the existing codemods to include this rename. Also as an outsider the documentation for the codemods is very limited (aside from the migrating.md file).