Add Summary Detail Component #2232
Conversation
interactivellama
temporarily deployed
to
design-system-react-co-pr-2232
components/summary-detail/index.jsx
Outdated
| /** | ||
| * Text to be shown on badge on right of the title | ||
| */ | ||
| badgeText: PropTypes.string, |
There was a problem hiding this comment.
This should take a whole badge component. Since we don't have one yet, please make it badge and pass HTML for now. that way folks can control the theme, etc.
components/summary-detail/index.jsx
Outdated
| /** | ||
| * Title for the summary | ||
| */ | ||
| title: PropTypes.string, |
There was a problem hiding this comment.
title, string, and detail should all be node and string types
components/summary-detail/index.jsx
Outdated
| type="button" | ||
| className="slds-button slds-button_icon slds-m-right_x-small" | ||
| title={assistiveText.toggleButton} | ||
| onClick={() => this.setState({ isOpen: isNotOpen })} |
There was a problem hiding this comment.
You should always prefer controlled components over uncontrolled in this library, so we need to add an onToggleDetail.
interactivellama
temporarily deployed
to
design-system-react-co-pr-2232
…react into map-component # Conflicts: # components/index.js # components/site-stories.js
interactivellama
temporarily deployed
to
design-system-react-co-pr-2232
interactivellama
temporarily deployed
to
design-system-react-co-pr-2232
components/summary-detail/index.jsx
Outdated
| className="slds-summary-detail__content" | ||
| id={`${this.getId()}-details`} | ||
| > | ||
| <p>{this.props.detail}</p> |
There was a problem hiding this comment.
This is often going to more than one paragraph. Consumers should have to add their own paragraph tags.
components/summary-detail/index.jsx
Outdated
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| isOpen: this.props.isOpen, |
There was a problem hiding this comment.
This sets initial control from props.isOpen but the render is never updated. If you put the open state in the example, you will notice that it doesn't get updated.
If the user sets the isOpen prop, then it's value should be used in render.
Other components do this with an getIsOpen function, https://github.com/salesforce/design-system-react/blob/master/components/popover/popover.jsx#L288
this also means you can't set the default to closed, because then it will have a value of false and be defined.
|
Sorry on taking out quite some time out, as I had to catch up with my academics badly. I will work on this asap. :) |
|
I have made the suggested changes, please review @interactivellama |
|
This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you. |
Fixes #2231
Additional description
Adds Summary Detail Component as per SLDS
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fixhas been run and linting passes.components/component-docs.jsonCI tests pass (npm test).REVIEWER checklist (do not remove)
components/component-docs.jsontests.Required only if there are markup / UX changes
last-slds-markup-reviewinpackage.jsonand push.last-accessibility-review, topackage.jsonand push.npm run local-updatewithin locally cloned site repo to confirm the site will function correctly at the next release.