-
Notifications
You must be signed in to change notification settings - Fork 534
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
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 |
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add deprecation warnings for these types as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we need to add this classname 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Thank you, @SferaDev!
Closes #1393
Breadcrum
toBreadcrums
andBreadcrumItem
toBreadcrumsItem
with 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).