-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Accordion): support panel's title as custom element #1144 #1281
feat(Accordion): support panel's title as custom element #1144 #1281
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
==========================================
+ Coverage 99.74% 99.74% +<.01%
==========================================
Files 140 140
Lines 2390 2399 +9
==========================================
+ Hits 2384 2393 +9
Misses 6 6
Continue to review full report at Codecov.
|
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.
The
active: PropTypes.bool, | ||
title: PropTypes.string, | ||
title: customPropTypes.contentShorthand, |
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 this prop is changed to shorthand, which we'd love, then its value should be passed to:
AccordionTitle.create(title)
The create()
method needs to be added to the title and content components to support this. You can reference other simple components with shorthand prop for subcomponents and children, such us, the Header. The Card is a good example of a component that has many levels of children.
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.
Thanks - I didn't quite get that from contributing guidelines.
Corrected (hopefully correctly 😄)
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.
No worries, our shorthand API and tools were developed after the guide and the guide was, unfortunately, not adequately updated. Sorry about that!
}] | ||
const wrapper = mount(<Accordion panels={panels} />) | ||
|
||
expect(wrapper.childAt(0).containsMatchingElement(panels[0].title)).to.equal(true) |
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.
Oh, I've made a mistake - this should still have 'title' class added
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.
Fixed. Also added example to docs showcasing the feature.
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 left a few comments, LMK if you have any issues or questions. Also, much of our latest practices for docs are not well documented (my apologies) but I can help out if you like once these first round of changes are made.
@@ -43,17 +50,29 @@ export default class AccordionTitle extends Component { | |||
parent: 'Accordion', | |||
} | |||
|
|||
static create = createShorthandFactory(AccordionTitle, content => ({ content })) |
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.
This method will need to be placed after the class is created, otherwise, AccordionTitle will be undefined in this context:
class AccordionTitle extends Component {
// ...
}
AccordionTitle.create = createShorthandFactory(AccordionTitle, content => ({ content }))
There is also a common test for this:
AccordionTitle-test.js
common.implementsCreateMethod(AccordionTitle)
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.
Actually, AccordionTitle is defined in such scenario and has right value (as shown in babel example). Nevertheless I've placed it after the class is created because there was some issue with code coverage not seeing the line as covered.
handleClick = (e) => { | ||
const { onClick } = this.props | ||
|
||
if (onClick) onClick(e, this.props) | ||
} | ||
|
||
renderContent(content) { | ||
return isValidElement ? content : ( |
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.
The isValidElement
is missing its argument isValidElement(content)
, however, this can be removed entirely and the content
always rendered in place.
Also, we try to avoid all styling and leave this to the user's CSS theme of choice. Let's remove the extra wrapping div
and inline style by going with two explicit returns in the render() method:
if (!_.isNil(content)) {
return (
<ElementType {...rest} className={classes} onClick={this.handleClick}>
<Icon name='dropdown' />
{content}
</ElementType>
)
}
return (
<ElementType {...rest} className={classes} onClick={this.handleClick}>
{children}
</ElementType>
)
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.
Oh my. I've fixed isValidElement issue and left the logic intact (apart from inserting the div
) as I strongly believe users should have the ability to customise the title fully. Without the check for element if I pass
title: (
<div>
<Icon name='heart' />
What a lovely title
</div>
)
it will render two icons (dropdown and heart). On the other side when somebody passes text the dropdown icon should be rendered.
import faker from 'faker' | ||
import _ from 'lodash' | ||
|
||
const panels = _.times(3, () => ({ | ||
title: faker.lorem.sentence(), | ||
const CustomTitle = ({ text }) => ( |
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.
Would prefer we create a separate example that shows shorthand element usage and leave this one for shorthand string usage.
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.
Done, let me now if there should be any description or any other title (writing good documentation is really really hard 😄)
Hm, this now assumes that all elements will always include an Icon. If I pass an element, such as a I also think the shorthand API should always be consistent (always icon or never icon). The majority use case would be to include the icon, presumably. I fear confusing users if the accordion sometimes has an icon and sometimes doesn't with no explicit visible reason as to why that would be the case (vs something like an icon prop). The idea with shorthand is to cover the majority of use cases while leaving the subcomponent API for the detailed use cases. If users are importing Icon and building a complete title, I'd direct them to use LMK your thoughts on this. |
content: (<CustomContent content={faker.lorem.sentence()} />), | ||
})) | ||
|
||
const AccordionExamplePanelsProp = () => ( |
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.
The default export should match the filename. Also, this would be a "Usage" example .../Examples/modules/Accordion/Usage
. We put all our SUIR specific examples here, for now. We reserve the types/states/variation/etc. for SUI core parity examples.
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.
Corrected 😄
Yeah, you're probably right. It does sound confusing when you put it that way.
Using Accordion.Title would be lovely, unfortunately you can't easily render them programatically unless there is Accordion.Panel (at least yet, tough any solution of implementing Accordion.Panel seems hacky). Altogether, I've changed the component so that it always renders the icon. |
…nt to Accordion docs" This reverts commit 1b84e22.
eaff805
to
146eb5e
Compare
I've just rebased this to the latest master and pushed a couple minor fixes. Will merge on pass. |
Released in |
…#1144 (Semantic-Org#1281) * feat(Accordion): support panel's title as custom element Semantic-Org#1144 * refactor(Accordion) panels created using createShorthandFactory * fix(Accordion) title and content custom elements not wrapped in div with class names * docs(Accordion) add example use case for custom title component to Accordion docs * refactor(Accordion): remove unused import * Revert "docs(Accordion) add example use case for custom title component to Accordion docs" This reverts commit 1b84e22. * fix(Accordion): Accordion Title does not have dropdown for panels with string title * docs(Accordion): added separate example for Accordion with custom title and content * feat(Accordion): Accordion consistently renders dropdown icon for panels' titles * docs(Accordion): Move custom title and content example to Usages * docs(Accordion): reorganize panels shorthand * feat(Accordion): support all child keys in panels
Short term fix for #1144
I've also added custom key to panels object named
key
because React is throwing errors when using own components as title or content. Usingpanel.title
as source of component key at<AccordionTitle key={`${panel.title}-title`} active={isActive} onClick={onClick}>
obviously does not work in such scenario aspanel.title
becomes[Object object]
when cast to string.