feat(BreadcrumbItem): pass props to li element #5055
Conversation
| @@ -5137,17 +5137,10 @@ functional-red-black-tree@^1.0.1: | |||
| resolved "https://registry.yarnpkg.com/functional-red-black-tree/-/functional-red-black-tree-1.0.1.tgz#1b0ab3bd553b2a0d6399d29c0e3ea0b252078327" | |||
| integrity sha1-GwqzvVU7Kg1jmdKcDj6gslIHgyc= | |||
|
|
|||
| <<<<<<< HEAD | |||
There was a problem hiding this comment.
there was a merge conflict merged by jquense
src/BreadcrumbItem.js
Outdated
| const { href, title, target, listItemProps, ...elementProps } = props; | ||
| const linkProps = { href, title, target }; | ||
|
|
||
| return ( | ||
| <Component | ||
| ref={ref} | ||
| className={classNames(prefix, className, { active })} | ||
| aria-current={active ? 'page' : undefined} | ||
| {...listItemProps} |
There was a problem hiding this comment.
| const { href, title, target, listItemProps, ...elementProps } = props; | |
| const linkProps = { href, title, target }; | |
| return ( | |
| <Component | |
| ref={ref} | |
| className={classNames(prefix, className, { active })} | |
| aria-current={active ? 'page' : undefined} | |
| {...listItemProps} | |
| const { href, title, target, ...props } = props; | |
| const linkProps = { href, title, target }; | |
| return ( | |
| <Component | |
| ref={ref} | |
| {...props} | |
| className={classNames(prefix, className, { active })} | |
| aria-current={active ? 'page' : undefined} |
maybe we can just do this?
There was a problem hiding this comment.
would break backwards compatibility right? thats why I made this approach.
There was a problem hiding this comment.
we're in betas :D
post-#5043 i think it's a reasonable change to make...
There was a problem hiding this comment.
maybe we should release v1 soon, so that we have at least something for Bootstrap 4.
Then for Bootstrap 5 we can use v2 or instantly jump to v5 idk. (seems to be the user friendly approach for me)
There was a problem hiding this comment.
yeah... i guess my concern with matching bootstrap major versions is that it doesn't let us make potentially necessary breaking changes on our end.
part of the question might be how often upstream cuts breaking changes. if it will happen on a semi-regular cadence, then not too big a deal.
if it will be years and years, then we have a bit of a problem
test/BreadcrumbItemSpec.js
Outdated
| it('Should be able to pass attributes to the link element', () => { | ||
| const instance = mount( | ||
| <Breadcrumb.Item linkProps={{ foo: 'bar' }}>Crumb</Breadcrumb.Item>, | ||
| ); | ||
| instance | ||
| .find('a') | ||
| .prop('foo') | ||
| .should.eq('bar'); | ||
| }); | ||
|
|
||
| it('Should be able to pass attributes to the li element', () => { | ||
| const instance = mount(<Breadcrumb.Item foo="bar">Crumb</Breadcrumb.Item>); | ||
| instance | ||
| .find('li') | ||
| .prop('foo') | ||
| .should.eq('bar'); | ||
| }); |
There was a problem hiding this comment.
Could we re-write these tests to make assertions on actual use cases?
There was a problem hiding this comment.
you mean dont use should.eq?
I adjusted the test names
types/components/BreadcrumbItem.d.ts
Outdated
| @@ -8,6 +8,7 @@ export interface BreadcrumbItemProps { | |||
| linkAs?: React.ElementType; | |||
| target?: string; | |||
| title?: React.ReactNode; | |||
| linkProps?: React.LinkHTMLAttributes<any>; | |||
There was a problem hiding this comment.
Is there a better generic that we can use here other than any?
There was a problem hiding this comment.
should be okay now. thx
Co-Authored-By: Jimmy Jia <[email protected]>
…tstrap into feature/breadcrumbitem-pass-props
|
Should be okay now. But unsure regarding the |
|
@jquense last blocker for v1 here |
Closes #5054