-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add Breadcrumb component #321
Add Breadcrumb component #321
Conversation
Current coverage is 91.51%@@ master #321 diff @@
==========================================
Files 59 62 +3
Lines 738 778 +40
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 672 712 +40
Misses 66 66
Partials 0 0
|
Thanks for the PR, it's looking great so far! It looks like you've probably already read the contributing guidelines. Looking forward to the API spec for this component. The CONTRIBUTING.md guide details how to go about designing a component API. The Spec out the API section is of primary importance. You can also reference other PRs for example specs, like #281. Cheers! |
Breadcrumb API ProposalBreadcrumbRegarding Semantic UI docs, Breadcrumb can have only variation, so I think there will no be <Breadcrumb>...</Breadcrumb>
<Breadcrumb size='massive'>...</Breadcrumb> Each breadcrumb contains sections and dividers. SectionThere is a trouble with section, it can be text or link, so I propose to add <Breadcrumb.Section>Home</Breadcrumb.Section>
<Breadcrumb.Section active>Home</Breadcrumb.Section>
<Breadcrumb.Section isLink>Home</Breadcrumb.Section>
<Breadcrumb.Section active isLink>Home</Breadcrumb.Section> DividerBreadcrumb component also has divider, but it's not <span class="divider">/</span>
<i class="right chevron icon divider"></i> I think of such an implementation: <Breadcrumb.Divider>/</Breadcrumb.Divider>
<Breadcrumb.Divider icon='right chevron'>/</Breadcrumb.Divider> All together<Breadcrumb>
<Breadcrumb.Section isLink>Home</Breadcrumb.Section>
<Breadcrumb.Divider icon='right chevron'>/</Breadcrumb.Divider>
<Breadcrumb.Section active>Catalog</Breadcrumb.Section>
</Breadcrumb> Feedback welcome 😄 |
I think you're right about the sub components and props here. Here are my thoughts. SectionSince props without a value default to I think we can do a similar thing here with the Breadcrumb. I see 3 ways of formatting the Section as a link:
If any of these three are present, I think we should use an <Breadcrumb.Section link>Home</Breadcrumb.Section>
<Breadcrumb.Section onClick={this.handleClick}>Home</Breadcrumb.Section>
<Breadcrumb.Section href='google.com'>Home</Breadcrumb.Section> <a class="section">Home</a>
<a class="section">Home</a>
<a class="section" href="google.com">Home</a> DividerThe default divider is <Breadcrumb.Divider />
// and
<Breadcrumb.Divider icon='right chevron' /> <div class="divider"> / </div>
<i class="right chevron icon divider"></i>
Icon
<Icon name='right chevron' divider /> <i class="right chevron icon divider"></i> See #279 for that component update. I'll /cc @jamiehill for this addition. Using PropsWe've also been allowing components to define basic markup through props. We would make I can see allowing something like this: const sections = []
<Breadcrumb sections={sections} /> Regularconst sections = [
{ text: 'Home' },
{ text: 'Store' },
{ text: 'T-Shirt', active: true }
]
<Breadcrumb sections={sections} /> <div class="ui breadcrumb">
<a class="section">Home</a>
<div class="divider"> / </div>
<a class="section">Store</a>
<div class="divider"> / </div>
<div class="active section">T-Shirt</div>
</div> IconsWe could use a const sections = [
{ text: 'Home' },
{ text: 'Store' },
{ text: 'T-Shirt', active: true }
]
<Breadcrumb sections={sections} divider='right angle' /> <div class="ui breadcrumb">
<a class="section">Home</a>
<i class="right angle icon divider"></i>
<a class="section">Store</a>
<i class="right angle icon divider"></i>
<div class="active section">T-Shirt</div>
</div> LinksJust as with the Breadcrumb.Section, you could provide a const sections = [
{ text: 'Home', link: true },
{ text: 'Store', onClick: handleClick },
{ text: 'T-Shirt', href: 'google.com' }
]
<Breadcrumb sections={sections} /> <div class="ui breadcrumb">
<a class="section">Home</a>
<i class="divider"></i>
<a class="section">Store</a>
<i class="divider"></i>
<div class="active" href='google.com'>T-Shirt</div>
</div> Let me know what you think! |
SectionI fully agree with your thoughts 👍 DividerI agree with default value, but it may to be set custom: <div class="divider">/</div>
<div class="divider">></div> <Breadcrumb.Divider />
<Breadcrumb.Divider delimiter='>' />
<Breadcrumb.Divider symbol='>' />
<Breadcrumb.Divider value='>' /> May be one of variants? IconI think better will be extend <Breadcrumb.Icon name='right chevron' />
// or
<Breadcrumb.IconDivider name='right chevron' /> Using propsI think this will be more prefered: <Breadcrumb sections={sections} divider='>' />
<Breadcrumb sections={sections} icon-divider='right angle' /> |
I like this but think we should use children instead of a delimiter, symbol, or value prop. It will also be more flexible incase someone would like to pass another component as the divider child. For the Icon, I like <Breadcrumbs icon name='right angle' /> This can just be an Icon where we add the "divider" className. I'm on mobile right now and it's a holiday here. If you'd like to start with this much, we can iterate on the rest. I'll be offline for the rest of the day. Thanks! |
Just noticed I copied the wrong JSX in my last comment. I meant to say for the Breadcrumb.Icon I like the idea of re-using the Icon component with this syntax: <Breadcrumb.Icon name='right angle' /> Where this would be exposed like so: // BreadcrumbIcon.js
import Icon from '../../elements/Icon/Icon'
function BreadcrumbIcon(props) {
// add the 'divider' class to the Icon
} // Breadcrumb.js
import BreadcrumbIcon from './BreadcrumbIcon'
function Breadcrumb(props) { /* ... */ }
Breadcrumb.Icon = BreadcrumbIcon |
Seems to me inconsistent with other Apis, Button for example, eg where icon is an Icon instance or icon class name, no? Sent from my iPhone
|
This was my original intuition as well. I've not given this one a fair amount of time or thought today. Starting tomorrow, I'll be able to look more at this. Thanks for the input and concern for the APIs. |
Breadcrumb.SectionI added component that implements described functionality. But here is problem with props. I think that only one prop ( Breadcrumb.DividerI think about this, what about this? <Breadcrumb.Divider />
<Breadcrumb.Divider>/</Breadcrumb.Divider>
<Breadcrumb.Divider icon='right chevron' />
<Breadcrumb sections={sections} />
<Breadcrumb sections={sections} divider='/' />
<Breadcrumb sections={sections} icon-divider='right angle' />
<Breadcrumb sections={sections} icon divider='right angle' /> |
I can see link and href being mutually exclusive, but I don't think the onClick should be. A user may want a click handler along with link/href.
This is a short coming of static propTypes = {
link: customPropTypes.mutuallyExclusive(['href'], PropTypes.bool),
href: customPropTypes.mutuallyExclusive(['link'], PropTypes.string),
} |
For the divider, how about this?: <Breadcrumb.Divider />
<Breadcrumb.Divider>/</Breadcrumb.Divider>
<Breadcrumb.Divider icon='right chevron' />
<Breadcrumb sections={sections} />
<Breadcrumb sections={sections} divider='/' />
<Breadcrumb sections={sections} icon='right angle' /> |
I just merged #323 which adds the panels: customPropTypes.all([
customPropTypes.mutuallyExclusive(['children']),
PropTypes.arrayOf(PropTypes.shape({
active: PropTypes.bool,
title: PropTypes.string,
content: PropTypes.string,
onClick: PropTypes.func,
})),
]), |
if (!_.isFunction(onClick)) return | ||
|
||
onClick() | ||
} |
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.
Few notes on event handlers.
- Never preventDefault (the user can prevent default if they want)
- Always pass back the event
- propTypes validates the onClick as being a function for us, so we don't have to include validation
The refactored handleClick
would look like this:
const handleClick = (e) => {
if (onClick) onClick(e)
}
Since now all this function does is pass the event from one function to another, it can be removed entirely. We can just spread the onClick
prop on the returned component. When called, it will also be called with the event.
There are times that we want to first capture a click handler. This would be when we want to pass some additional arguments after the event argument.
Before I forget, go ahead and check off Breadcrumb in the README.md under support. |
…into feature-breadcrumb
@@ -0,0 +1,48 @@ | |||
import React from 'react' | |||
import Breadcrumb from 'src/collections/Breadcrumb/Breadcrumb' |
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 import the actual component here so we don't have a dependency on how it is exposed to the user.
import BreadcrumbSection from 'src/collections/Breadcrumb/BreadcrumbSection'
It's okay that following JSX will be generate this HTML? <Breadcrumb.Section link>Home</Breadcrumb.Section> <a class="section" href="javascript:void(0)">Home</a> May be better solution? |
@@ -6,46 +6,26 @@ import sandbox from 'test/utils/Sandbox-util' | |||
describe('BreadcrumbSection', () => { | |||
common.isConformant(Breadcrumb.Section) | |||
common.rendersChildren(Breadcrumb.Section) | |||
common.propValueOnlyToClassName(Breadcrumb.Section, 'active') |
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 you point me? Why test is failed?
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 believe you meant to use key only instead of value only:
-common.propValueOnlyToClassName(Breadcrumb.Section, 'active')
+common.propKeyOnlyToClassName(Breadcrumb.Section, 'active')
Key only will use the key name (active
) as the className whereas value only will use the value. Example, color='red'
uses value only, since we only the value red
as the className.
Also, it is helpful to run npm run test:watch
and use exclusive tests to reduce the clutter and identify issues.
Lastly, you can always click details
on the ci/circle ci check on the PR to see the issue: https://circleci.com/gh/TechnologyAdvice/stardust/1315
const wrapper = mount(<Breadcrumb sections={sections} divider='>' />) | ||
const divider = wrapper.find('BreadcrumbDivider').first() | ||
|
||
divider.text().should.to.equal('>') |
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.
We use the chai-enzyme plugin. This allows us to move the enzyme method to the end of the assertion chain:
-divider.text().should.to.equal('>')
+divider.should.contain.text('>')
This makes for much more readable errors as well. Compare the assertion syntax here with the error messages:
shallow(<Button>foo</Button>)
.text().should.equal('bar')
//=> expected 'foo' to equal 'bar'
shallow(<Button>foo</Button>)
.should.contain.text('bar')
//=> expected <Button /> to contain text 'bar', but it has 'foo'
HTML:
<button type="button" class="ui button">foo</button>
I made copy for |
<ExampleSection title='Content'> | ||
<ComponentExample | ||
title='Divider' | ||
description='A breadcrumb can contain a divider to show the relationship between sections, this can be |
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.
Strings in JS cannot span more than one line unless you use backticks, but this preserves whitespace including line breaks. Suggest using an array and joining the lines:
description={[
'A breadcrumb can contain a divider to show the relationship between sections,',
'this can be formatted as an icon or text.',
].join(' ')}
Wow, nice work on the docs. Very thankful for all your PRs! Left a couple comments on the docs. I'll see what we can do to get the Icon merged quickly so we can merge this one too 👍 |
I think we can wait until Icon's PR will be merged 😄 |
Icon may have some more doc refactors coming. How about we update this PR and merge it to keep pace? EDIT I'd hate to hold up this great work for a single use of the Icon :) |
@levithomason I've that |
Hoping to get the conflict resolved! https://github.com/TechnologyAdvice/stardust/pull/279#issuecomment-232087055 |
// TODO: After update <Icon> to API replace with this code: | ||
// return <Icon className={classes} name={icon} {...rest} /> | ||
|
||
return <Icon className={[icon, classes].join(' ')} {...rest} /> |
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've updated usage of Icon
, but it's still failing common.implementsIconProp
. It seems that this test it's not correct for such situation.
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.
Icon/Image props need to support both strings and components being passed. We use the iconPropRenderer
util for this. This case is a little more tricky because we also need to add the users classes as well as the divider
class to the icon after it is rendered.
The common test was over asserting things, I've updated it. I've also pushed an update the prop renderers to support passing props. I've tested this with the new renderers and it works:
-import { customPropTypes, getUnhandledProps } from '../../utils/propUtils'
-import Icon from 'src/elements/Icon/Icon'
+import { customPropTypes, getUnhandledProps, iconPropRenderer } from '../../utils/propUtils'
/**
* A divider sub-component for Breadcrumb component.
*/
function BreadcrumbDivider(props) {
const {
children, icon, className,
} = props
const classes = cx(
className,
'divider',
)
const rest = getUnhandledProps(BreadcrumbDivider, props)
if (icon) {
// TODO: After update <Icon> to API replace with this code:
// return <Icon className={classes} name={icon} {...rest} />
- return <Icon className={[icon, classes].join(' ')} {...rest} />
+ const iconClasses = _.isString(icon)
+ ? cx(icon, classes)
+ : cx(icon.props.className, classes)
+
+ return iconPropRenderer(icon, { ...rest, className: iconClasses })
}
Note I also used cx
to build up the icon classes.
ping @levithomason |
Hmm, I'll pull this and see what's going on. |
…into feature-breadcrumb
🐺 |
Fantastic, merging! Side note, I'm going to also extend the prop renderers to merge the className when a component is passed. This way, we don't have to check if |
* Flag component * Flag component Semantic-Org#322 * README.md update Semantic-Org#322 * Flag Component test update Semantic-Org#321
* Breadcrumb component * Breadcrumb.Section Semantic-Org#321 * Breadcrumb.Section Semantic-Org#321 * README.md update Semantic-Org#321 * (refactor) Breadcrumb.Section Semantic-Org#321 * (fix) Breadcrumb.Section test Semantic-Org#321 * (fix) Breadcrumb.Section Semantic-Org#321 * (fix) Breadcrumb.Section Semantic-Org#321 * (feat) Breadcrumb.Section Semantic-Org#321 * (fix) Breadcrumb.Divider Semantic-Org#321 * (feat) Breadcrumb Semantic-Org#321 * (fix) Breadcrumb.Section test Semantic-Org#321 * (fix) Breadcrumb.Section test Semantic-Org#321 * (fix) Breadcrumb Semantic-Org#321 * (feat) Breadcrumb Semantic-Org#321 * (fix) Breadcrumb key Semantic-Org#321 * (feat) Breadcrumb test Semantic-Org#321 * (feat) Breadcrumb tests Semantic-Org#321 * (feat) Breadcrumb docs Semantic-Org#321 * (fix) Breadcrumb docs Semantic-Org#321 * (fix) Breadcrumb Semantic-Org#321 * (fix) Breadcrumb docs * (fix) Breadcrumb * (Breadcrumb) Test update
Under Construction
Fixes #184