From 99b6a571108c95c52d18fdc9f322e984343f68db Mon Sep 17 00:00:00 2001 From: Malcolm Date: Tue, 22 May 2018 06:48:10 +1200 Subject: [PATCH] feat(factories): add autoGenerateKey option (#2424) * feat(factories): add `generateKey` option Can be used to disable automatic key generation for shorthand props with string/number values when the factory is called (not when it is created). * Rename generateKey to autoGenerateKey * Refactor tests * Add failing shorthand prop tests * Clean up JSDoc * `autoGenerateKey: false` flag added for shorthands that are tested, tests passing * fix(Confirm): fix shorthand key generation * fix(Label): fix shorthand key generation * feat(LabelDetail): add create method * fix(Step): fix shorthand key generation * fix(List): fix shorthand key generation * fix(Popup): fix shorthand key generation * fix(Progress): fix shorthand key generation * fix(Search): fix shorthand key generation * fix(Tab): fix shorthand key generation * fix(Card): fix shorthand key generation * fix(CommentAvatar): fix shorthand key generation * feat(docs): improve ux * fix(Tab): restore tab pane keys --- .../ComponentExample/ComponentExample.js | 2 + .../Confirm/Types/ConfirmExampleConfirm.js | 13 ++--- .../ModalExampleScrollingContent.js | 7 +-- src/addons/Confirm/Confirm.js | 25 +++------ .../Breadcrumb/BreadcrumbDivider.js | 8 ++- src/collections/Form/FormField.js | 11 +++- src/collections/Menu/MenuItem.js | 2 +- src/collections/Message/Message.js | 8 +-- src/elements/Button/Button.js | 5 +- src/elements/Header/Header.js | 6 +- src/elements/Image/Image.js | 4 +- src/elements/Input/Input.js | 7 ++- src/elements/Label/Label.js | 55 ++++++++++--------- src/elements/Label/LabelDetail.js | 3 + src/elements/List/ListItem.js | 45 ++++++++++++--- src/elements/Step/Step.js | 16 ++++-- src/elements/Step/StepContent.js | 4 +- src/lib/factories.js | 4 +- src/modules/Checkbox/Checkbox.js | 4 +- src/modules/Dropdown/Dropdown.js | 3 +- src/modules/Dropdown/DropdownHeader.js | 2 +- src/modules/Dropdown/DropdownItem.js | 12 ++-- src/modules/Embed/Embed.js | 3 +- src/modules/Modal/Modal.js | 4 +- src/modules/Popup/Popup.js | 29 +++++----- src/modules/Progress/Progress.js | 35 ++++-------- src/modules/Search/Search.js | 23 ++++++-- src/modules/Search/SearchResult.js | 25 +++------ src/modules/Tab/Tab.js | 20 ++++--- src/views/Card/Card.js | 14 ++++- src/views/Card/CardContent.js | 6 +- src/views/Comment/CommentAvatar.js | 6 +- src/views/Feed/FeedContent.js | 10 ++-- src/views/Feed/FeedEvent.js | 4 +- src/views/Feed/FeedLabel.js | 2 +- src/views/Feed/FeedLike.js | 2 +- src/views/Feed/FeedMeta.js | 2 +- src/views/Feed/FeedSummary.js | 4 +- src/views/Item/Item.js | 2 +- src/views/Item/ItemContent.js | 8 +-- src/views/Statistic/Statistic.js | 3 +- .../commonTests/implementsShorthandProp.js | 15 ++++- test/specs/elements/Label/LabelDetail-test.js | 1 + test/specs/lib/factories-test.js | 27 ++++++++- 44 files changed, 295 insertions(+), 196 deletions(-) diff --git a/docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js b/docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js index f16c586374..3ea71dd750 100644 --- a/docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js +++ b/docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js @@ -311,6 +311,8 @@ class ComponentExample extends PureComponent { } }, 100) + getComponentName = () => this.props.examplePath.split('/')[1] + handleChangeCode = (sourceCode) => { this.setState({ sourceCode }, this.renderSourceCode) } diff --git a/docs/app/Examples/addons/Confirm/Types/ConfirmExampleConfirm.js b/docs/app/Examples/addons/Confirm/Types/ConfirmExampleConfirm.js index 9d3eb55648..bda346d6ea 100644 --- a/docs/app/Examples/addons/Confirm/Types/ConfirmExampleConfirm.js +++ b/docs/app/Examples/addons/Confirm/Types/ConfirmExampleConfirm.js @@ -4,19 +4,14 @@ import { Button, Confirm } from 'semantic-ui-react' class ConfirmExampleConfirm extends Component { state = { open: false } - show = () => this.setState({ open: true }) - handleConfirm = () => this.setState({ open: false }) - handleCancel = () => this.setState({ open: false }) + open = () => this.setState({ open: true }) + close = () => this.setState({ open: false }) render() { return (
- - + +
) } diff --git a/docs/app/Examples/modules/Modal/Variations/ModalExampleScrollingContent.js b/docs/app/Examples/modules/Modal/Variations/ModalExampleScrollingContent.js index 5f4f78cbe8..2e41b63cd7 100644 --- a/docs/app/Examples/modules/Modal/Variations/ModalExampleScrollingContent.js +++ b/docs/app/Examples/modules/Modal/Variations/ModalExampleScrollingContent.js @@ -6,11 +6,7 @@ const ModalExampleScrollingContent = () => ( Scrolling Content Modal}> Profile Picture - +
Modal Header
@@ -34,4 +30,3 @@ const ModalExampleScrollingContent = () => ( ) export default ModalExampleScrollingContent - diff --git a/src/addons/Confirm/Confirm.js b/src/addons/Confirm/Confirm.js index f623c225cb..e7a85737ca 100644 --- a/src/addons/Confirm/Confirm.js +++ b/src/addons/Confirm/Confirm.js @@ -2,11 +2,7 @@ import _ from 'lodash' import PropTypes from 'prop-types' import React, { Component } from 'react' -import { - customPropTypes, - getUnhandledProps, - META, -} from '../../lib' +import { customPropTypes, getUnhandledProps, META } from '../../lib' import Button from '../../elements/Button' import Modal from '../../modules/Modal' @@ -82,14 +78,7 @@ class Confirm extends Component { }) render() { - const { - cancelButton, - confirmButton, - content, - header, - open, - size, - } = this.props + const { cancelButton, confirmButton, content, header, open, size } = this.props const rest = getUnhandledProps(Confirm, this.props) // `open` is auto controlled by the Modal @@ -100,11 +89,15 @@ class Confirm extends Component { return ( - {Modal.Header.create(header)} - {Modal.Content.create(content)} + {Modal.Header.create(header, { autoGenerateKey: false })} + {Modal.Content.create(content, { autoGenerateKey: false })} - {Button.create(cancelButton, { overrideProps: this.handleCancelOverrides })} + {Button.create(cancelButton, { + autoGenerateKey: false, + overrideProps: this.handleCancelOverrides, + })} {Button.create(confirmButton, { + autoGenerateKey: false, defaultProps: { primary: true }, overrideProps: this.handleConfirmOverrides, })} diff --git a/src/collections/Breadcrumb/BreadcrumbDivider.js b/src/collections/Breadcrumb/BreadcrumbDivider.js index 363069702b..0b46bb32ac 100644 --- a/src/collections/Breadcrumb/BreadcrumbDivider.js +++ b/src/collections/Breadcrumb/BreadcrumbDivider.js @@ -28,7 +28,13 @@ function BreadcrumbDivider(props) { const rest = getUnhandledProps(BreadcrumbDivider, props) const ElementType = getElementType(BreadcrumbDivider, props) - if (!_.isNil(icon)) return Icon.create(icon, { defaultProps: { ...rest, className: classes } }) + if (!_.isNil(icon)) { + return Icon.create(icon, { + defaultProps: { ...rest, className: classes }, + autoGenerateKey: false, + }) + } + if (!_.isNil(content)) return {content} return ( diff --git a/src/collections/Form/FormField.js b/src/collections/Form/FormField.js index 20244a05d1..77dbe5f2cc 100644 --- a/src/collections/Form/FormField.js +++ b/src/collections/Form/FormField.js @@ -68,7 +68,11 @@ function FormField(props) { ) } - return {createHTMLLabel(label)} + return ( + + {createHTMLLabel(label, { autoGenerateKey: false })} + + ) } // ---------------------------------------- @@ -102,8 +106,9 @@ function FormField(props) { return ( - {createHTMLLabel(label, { defaultProps: { - htmlFor: _.get(controlProps, 'id') }, + {createHTMLLabel(label, { + defaultProps: { htmlFor: _.get(controlProps, 'id') }, + autoGenerateKey: false, })} {createElement(control, controlProps)} diff --git a/src/collections/Menu/MenuItem.js b/src/collections/Menu/MenuItem.js index 021e73bbab..81e47c323b 100644 --- a/src/collections/Menu/MenuItem.js +++ b/src/collections/Menu/MenuItem.js @@ -131,7 +131,7 @@ export default class MenuItem extends Component { return ( - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {childrenUtils.isNil(content) ? _.startCase(name) : content} ) diff --git a/src/collections/Message/Message.js b/src/collections/Message/Message.js index 856cdce227..4247b27252 100644 --- a/src/collections/Message/Message.js +++ b/src/collections/Message/Message.js @@ -178,12 +178,12 @@ export default class Message extends Component { return ( {dismissIcon} - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {(!_.isNil(header) || !_.isNil(content) || !_.isNil(list)) && ( - {MessageHeader.create(header)} - {MessageList.create(list)} - {createHTMLParagraph(content)} + {MessageHeader.create(header, { autoGenerateKey: false })} + {MessageList.create(list, { autoGenerateKey: false })} + {createHTMLParagraph(content, { autoGenerateKey: false })} )} diff --git a/src/elements/Button/Button.js b/src/elements/Button/Button.js index b3b48b1bf1..a27b54b464 100644 --- a/src/elements/Button/Button.js +++ b/src/elements/Button/Button.js @@ -253,13 +253,14 @@ class Button extends Component { basic: true, pointing: labelPosition === 'left' ? 'right' : 'left', }, + autoGenerateKey: false, }) return ( {labelPosition === 'left' && labelElement} {(labelPosition === 'right' || !labelPosition) && labelElement} @@ -280,7 +281,7 @@ class Button extends Component { tabIndex={tabIndex} > {hasChildren && children} - {!hasChildren && Icon.create(icon)} + {!hasChildren && Icon.create(icon, { autoGenerateKey: false })} {!hasChildren && content} ) diff --git a/src/elements/Header/Header.js b/src/elements/Header/Header.js index 5c151cea43..f608a6bf52 100644 --- a/src/elements/Header/Header.js +++ b/src/elements/Header/Header.js @@ -68,9 +68,9 @@ function Header(props) { return {children} } - const iconElement = Icon.create(icon) - const imageElement = Image.create(image) - const subheaderElement = HeaderSubheader.create(subheader) + const iconElement = Icon.create(icon, { autoGenerateKey: false }) + const imageElement = Image.create(image, { autoGenerateKey: false }) + const subheaderElement = HeaderSubheader.create(subheader, { autoGenerateKey: false }) if (iconElement || imageElement) { return ( diff --git a/src/elements/Image/Image.js b/src/elements/Image/Image.js index 8611cd25c6..9d7a0ed102 100644 --- a/src/elements/Image/Image.js +++ b/src/elements/Image/Image.js @@ -86,8 +86,8 @@ function Image(props) { if (ElementType === 'img') return return ( - {Dimmer.create(dimmer)} - {Label.create(label)} + {Dimmer.create(dimmer, { autoGenerateKey: false })} + {Label.create(label, { autoGenerateKey: false })} ) diff --git a/src/elements/Input/Input.js b/src/elements/Input/Input.js index e4c5a4cb1c..6a343c5ffc 100644 --- a/src/elements/Input/Input.js +++ b/src/elements/Input/Input.js @@ -221,7 +221,7 @@ class Input extends Component { // Render Shorthand // ---------------------------------------- - const actionElement = Button.create(action) + const actionElement = Button.create(action, { autoGenerateKey: false }) const labelElement = Label.create(label, { defaultProps: { className: cx( @@ -230,15 +230,16 @@ class Input extends Component { _.includes(labelPosition, 'corner') && labelPosition, ), }, + autoGenerateKey: false, }) return ( {actionPosition === 'left' && actionElement} {labelPosition !== 'right' && labelElement} - {createHTMLInput(input || type, { defaultProps: htmlInputProps })} + {createHTMLInput(input || type, { defaultProps: htmlInputProps, autoGenerateKey: false })} {actionPosition !== 'left' && actionElement} - {Icon.create(this.computeIcon())} + {Icon.create(this.computeIcon(), { autoGenerateKey: false })} {labelPosition === 'right' && labelElement} ) diff --git a/src/elements/Label/Label.js b/src/elements/Label/Label.js index db5cbd8870..598b440bfa 100644 --- a/src/elements/Label/Label.js +++ b/src/elements/Label/Label.js @@ -5,7 +5,6 @@ import React, { Component } from 'react' import { childrenUtils, - createShorthand, createShorthandFactory, customPropTypes, getElementType, @@ -33,7 +32,14 @@ export default class Label extends Component { active: PropTypes.bool, /** A label can attach to a content segment. */ - attached: PropTypes.oneOf(['top', 'bottom', 'top right', 'top left', 'bottom left', 'bottom right']), + attached: PropTypes.oneOf([ + 'top', + 'bottom', + 'top right', + 'top left', + 'bottom left', + 'bottom right', + ]), /** A label can reduce its complexity. */ basic: PropTypes.bool, @@ -54,19 +60,13 @@ export default class Label extends Component { content: customPropTypes.contentShorthand, /** A label can position itself in the corner of an element. */ - corner: PropTypes.oneOfType([ - PropTypes.bool, - PropTypes.oneOf(['left', 'right']), - ]), + corner: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['left', 'right'])]), /** Shorthand for LabelDetail. */ detail: customPropTypes.itemShorthand, /** Formats the label as a dot. */ - empty: customPropTypes.every([ - PropTypes.bool, - customPropTypes.demand(['circular']), - ]), + empty: customPropTypes.every([PropTypes.bool, customPropTypes.demand(['circular'])]), /** Float above another element in the upper right corner. */ floating: PropTypes.bool, @@ -78,10 +78,7 @@ export default class Label extends Component { icon: customPropTypes.itemShorthand, /** A label can be formatted to emphasize an image or prop can be used as shorthand for Image. */ - image: PropTypes.oneOfType([ - PropTypes.bool, - customPropTypes.itemShorthand, - ]), + image: PropTypes.oneOfType([PropTypes.bool, customPropTypes.itemShorthand]), /** * Called on click. @@ -109,10 +106,7 @@ export default class Label extends Component { removeIcon: customPropTypes.itemShorthand, /** A label can appear as a ribbon attaching itself to an element. */ - ribbon: PropTypes.oneOfType([ - PropTypes.bool, - PropTypes.oneOf(['right']), - ]), + ribbon: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['right'])]), /** A label can have different sizes. */ size: PropTypes.oneOf(SUI.SIZES), @@ -167,9 +161,10 @@ export default class Label extends Component { tag, } = this.props - const pointingClass = (pointing === true && 'pointing') - || ((pointing === 'left' || pointing === 'right') && `${pointing} pointing`) - || ((pointing === 'above' || pointing === 'below') && `pointing ${pointing}`) + const pointingClass = + (pointing === true && 'pointing') || + ((pointing === 'left' || pointing === 'right') && `${pointing} pointing`) || + ((pointing === 'above' || pointing === 'below') && `pointing ${pointing}`) const classes = cx( 'ui', @@ -194,18 +189,26 @@ export default class Label extends Component { const ElementType = getElementType(Label, this.props) if (!childrenUtils.isNil(children)) { - return {children} + return ( + + {children} + + ) } const removeIconShorthand = _.isUndefined(removeIcon) ? 'delete' : removeIcon return ( - {Icon.create(icon)} - {typeof image !== 'boolean' && Image.create(image)} + {Icon.create(icon, { autoGenerateKey: false })} + {typeof image !== 'boolean' && Image.create(image, { autoGenerateKey: false })} {content} - {createShorthand(LabelDetail, val => ({ content: val }), detail)} - {onRemove && Icon.create(removeIconShorthand, { overrideProps: this.handleIconOverrides })} + {LabelDetail.create(detail, { autoGenerateKey: false })} + {onRemove && + Icon.create(removeIconShorthand, { + autoGenerateKey: false, + overrideProps: this.handleIconOverrides, + })} ) } diff --git a/src/elements/Label/LabelDetail.js b/src/elements/Label/LabelDetail.js index 032d734e92..1229bc4797 100644 --- a/src/elements/Label/LabelDetail.js +++ b/src/elements/Label/LabelDetail.js @@ -4,6 +4,7 @@ import React from 'react' import { childrenUtils, + createShorthandFactory, customPropTypes, getElementType, getUnhandledProps, @@ -43,4 +44,6 @@ LabelDetail.propTypes = { content: customPropTypes.contentShorthand, } +LabelDetail.create = createShorthandFactory(LabelDetail, val => ({ content: val })) + export default LabelDetail diff --git a/src/elements/List/ListItem.js b/src/elements/List/ListItem.js index 63b87f596a..62db6f8ec6 100644 --- a/src/elements/List/ListItem.js +++ b/src/elements/List/ListItem.js @@ -118,30 +118,51 @@ class ListItem extends Component { if (!childrenUtils.isNil(children)) { return ( - + {children} ) } - const iconElement = ListIcon.create(icon) - const imageElement = Image.create(image) + const iconElement = ListIcon.create(icon, { autoGenerateKey: false }) + const imageElement = Image.create(image, { autoGenerateKey: false }) // See description of `content` prop for explanation about why this is necessary. if (!isValidElement(content) && _.isPlainObject(content)) { return ( - + {iconElement || imageElement} - {ListContent.create(content, { header, description })} + {ListContent.create(content, { + autoGenerateKey: false, + defaultProps: { header, description }, + })} ) } - const headerElement = ListHeader.create(header) - const descriptionElement = ListDescription.create(description) + const headerElement = ListHeader.create(header, { autoGenerateKey: false }) + const descriptionElement = ListDescription.create(description, { autoGenerateKey: false }) if (iconElement || imageElement) { return ( - + {iconElement || imageElement} {(content || headerElement || descriptionElement) && ( @@ -155,7 +176,13 @@ class ListItem extends Component { } return ( - + {headerElement} {descriptionElement} {content} diff --git a/src/elements/Step/Step.js b/src/elements/Step/Step.js index bf62e9252d..5af35a1994 100644 --- a/src/elements/Step/Step.js +++ b/src/elements/Step/Step.js @@ -122,17 +122,25 @@ class Step extends Component { const ElementType = getElementType(Step, this.props, this.computeElementType) if (!childrenUtils.isNil(children)) { - return {children} + return ( + + {children} + + ) } if (!childrenUtils.isNil(content)) { - return {content} + return ( + + {content} + + ) } return ( - {Icon.create(icon)} - {StepContent.create({ description, title })} + {Icon.create(icon, { autoGenerateKey: false })} + {StepContent.create({ description, title }, { autoGenerateKey: false })} ) } diff --git a/src/elements/Step/StepContent.js b/src/elements/Step/StepContent.js index b29faf195a..15982159cd 100644 --- a/src/elements/Step/StepContent.js +++ b/src/elements/Step/StepContent.js @@ -27,8 +27,8 @@ function StepContent(props) { return ( - {StepTitle.create(title)} - {StepDescription.create(description)} + {StepTitle.create(title, { autoGenerateKey: false })} + {StepDescription.create(description, { autoGenerateKey: false })} ) } diff --git a/src/lib/factories.js b/src/lib/factories.js index d6fa616b85..8922ba05de 100644 --- a/src/lib/factories.js +++ b/src/lib/factories.js @@ -15,6 +15,7 @@ import React, { cloneElement, isValidElement } from 'react' * @param {Object} [options={}] * @param {object} [options.defaultProps={}] Default props object * @param {object|function} [options.overrideProps={}] Override props object or function (called with regular props) + * @param {boolean} [options.autoGenerateKey=true] Whether or not automatic key generation is allowed * @returns {object|null} */ export function createShorthand(Component, mapValueToProps, val, options = {}) { @@ -90,12 +91,13 @@ export function createShorthand(Component, mapValueToProps, val, options = {}) { // Use key, childKey, or generate key if (_.isNil(props.key)) { const { childKey } = props + const { autoGenerateKey = true } = options if (!_.isNil(childKey)) { // apply and consume the childKey props.key = typeof childKey === 'function' ? childKey(props) : childKey delete props.childKey - } else if (valIsString || valIsNumber) { + } else if (autoGenerateKey && (valIsString || valIsNumber)) { // use string/number shorthand values as the key props.key = val } diff --git a/src/modules/Checkbox/Checkbox.js b/src/modules/Checkbox/Checkbox.js index 9556b0efd1..355c851b60 100644 --- a/src/modules/Checkbox/Checkbox.js +++ b/src/modules/Checkbox/Checkbox.js @@ -251,7 +251,9 @@ export default class Checkbox extends Component { Heads Up! Do not remove empty labels, they are required by SUI CSS */} - {createHTMLLabel(label, { defaultProps: { htmlFor: id } }) || ) } diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js index 4a0c51e9ea..eee42a929b 100644 --- a/src/modules/Dropdown/Dropdown.js +++ b/src/modules/Dropdown/Dropdown.js @@ -1212,7 +1212,7 @@ export default class Dropdown extends Component { return ( - {DropdownHeader.create(header)} + {DropdownHeader.create(header, { autoGenerateKey: false })} {this.renderOptions()} ) @@ -1302,6 +1302,7 @@ export default class Dropdown extends Component { {trigger || this.renderText()} {Icon.create(icon, { overrideProps: this.handleIconOverrides, + autoGenerateKey: false, })} {this.renderMenu()} diff --git a/src/modules/Dropdown/DropdownHeader.js b/src/modules/Dropdown/DropdownHeader.js index 9bd1e44fe1..c0554ad30f 100644 --- a/src/modules/Dropdown/DropdownHeader.js +++ b/src/modules/Dropdown/DropdownHeader.js @@ -31,7 +31,7 @@ function DropdownHeader(props) { return ( - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {content} ) diff --git a/src/modules/Dropdown/DropdownItem.js b/src/modules/Dropdown/DropdownItem.js index bf9ea992f0..04cd3d6f93 100644 --- a/src/modules/Dropdown/DropdownItem.js +++ b/src/modules/Dropdown/DropdownItem.js @@ -135,21 +135,21 @@ class DropdownItem extends Component { ) } - const flagElement = Flag.create(flag) - const iconElement = Icon.create(iconName) - const imageElement = Image.create(image) - const labelElement = Label.create(label) + const flagElement = Flag.create(flag, { autoGenerateKey: false }) + const iconElement = Icon.create(iconName, { autoGenerateKey: false }) + const imageElement = Image.create(image, { autoGenerateKey: false }) + const labelElement = Label.create(label, { autoGenerateKey: false }) const descriptionElement = createShorthand( 'span', val => ({ children: val }), description, - { defaultProps: { className: 'description' } }, + { defaultProps: { className: 'description' }, autoGenerateKey: false }, ) const textElement = createShorthand( 'span', val => ({ children: val }), childrenUtils.isNil(content) ? text : content, - { defaultProps: { className: 'text' } }, + { defaultProps: { className: 'text' }, autoGenerateKey: false }, ) return ( diff --git a/src/modules/Embed/Embed.js b/src/modules/Embed/Embed.js index c534404134..317cbd12bd 100644 --- a/src/modules/Embed/Embed.js +++ b/src/modules/Embed/Embed.js @@ -179,7 +179,7 @@ export default class Embed extends Component { return ( - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {placeholder && } {this.renderEmbed()} @@ -206,6 +206,7 @@ export default class Embed extends Component { title: `Embedded content from ${source}.`, width: '100%', }, + autoGenerateKey: false, })} ) diff --git a/src/modules/Modal/Modal.js b/src/modules/Modal/Modal.js index 5103c7302e..f2a7a866f5 100644 --- a/src/modules/Modal/Modal.js +++ b/src/modules/Modal/Modal.js @@ -302,8 +302,8 @@ class Modal extends Component { {closeIconJSX} - {ModalHeader.create(header)} - {ModalContent.create(content)} + {ModalHeader.create(header, { autoGenerateKey: false })} + {ModalContent.create(content, { autoGenerateKey: false })} {ModalActions.create(actions, { overrideProps: this.handleActionsOverrides })} diff --git a/src/modules/Popup/Popup.js b/src/modules/Popup/Popup.js index e7900addf8..0ba189a2de 100644 --- a/src/modules/Popup/Popup.js +++ b/src/modules/Popup/Popup.js @@ -130,10 +130,7 @@ export default class Popup extends Component { trigger: PropTypes.node, /** Popup width. */ - wide: PropTypes.oneOfType([ - PropTypes.bool, - PropTypes.oneOf(['very']), - ]), + wide: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['very'])]), /** Element to be rendered within the confines of the viewport whenever possible. */ keepInViewPort: PropTypes.bool, @@ -172,7 +169,8 @@ export default class Popup extends Component { } else if (_.includes(positions, 'left')) { style.left = Math.round(coords.left + pageXOffset) style.right = 'auto' - } else { // if not left nor right, we are horizontally centering the element + } else { + // if not left nor right, we are horizontally centering the element const xOffset = (coords.width - this.popupCoords.width) / 2 style.left = Math.round(coords.left + xOffset + pageXOffset) style.right = 'auto' @@ -184,9 +182,10 @@ export default class Popup extends Component { } else if (_.includes(positions, 'bottom')) { style.top = Math.round(coords.bottom + pageYOffset) style.bottom = 'auto' - } else { // if not top nor bottom, we are vertically centering the element + } else { + // if not top nor bottom, we are vertically centering the element const yOffset = (coords.height + this.popupCoords.height) / 2 - style.top = Math.round((coords.bottom + pageYOffset) - yOffset) + style.top = Math.round(coords.bottom + pageYOffset - yOffset) style.bottom = 'auto' const xOffset = this.popupCoords.width + 8 @@ -393,19 +392,23 @@ export default class Popup extends Component { const unhandled = getUnhandledProps(Popup, this.props) const portalPropNames = Portal.handledProps - const rest = _.reduce(unhandled, (acc, val, key) => { - if (!_.includes(portalPropNames, key)) acc[key] = val + const rest = _.reduce( + unhandled, + (acc, val, key) => { + if (!_.includes(portalPropNames, key)) acc[key] = val - return acc - }, {}) + return acc + }, + {}, + ) const portalProps = _.pick(unhandled, portalPropNames) const ElementType = getElementType(Popup, this.props) const popupJSX = ( {children} - {childrenUtils.isNil(children) && PopupHeader.create(header)} - {childrenUtils.isNil(children) && PopupContent.create(content)} + {childrenUtils.isNil(children) && PopupHeader.create(header, { autoGenerateKey: false })} + {childrenUtils.isNil(children) && PopupContent.create(content, { autoGenerateKey: false })} ) diff --git a/src/modules/Progress/Progress.js b/src/modules/Progress/Progress.js index bb268daa66..57554a02de 100644 --- a/src/modules/Progress/Progress.js +++ b/src/modules/Progress/Progress.js @@ -62,20 +62,14 @@ class Progress extends Component { /** Current percent complete. */ percent: customPropTypes.every([ customPropTypes.disallow(['total', 'value']), - PropTypes.oneOfType([ - PropTypes.number, - PropTypes.string, - ]), + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), ]), /** Decimal point precision for calculated progress. */ precision: PropTypes.number, /** A progress bar can contain a text value indicating current progress. */ - progress: PropTypes.oneOfType([ - PropTypes.bool, - PropTypes.oneOf(['percent', 'ratio', 'value']), - ]), + progress: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['percent', 'ratio', 'value'])]), /** A progress bar can vary in size. */ size: PropTypes.oneOf(_.without(SUI.SIZES, 'mini', 'huge', 'massive')), @@ -87,19 +81,13 @@ class Progress extends Component { total: customPropTypes.every([ customPropTypes.demand(['value']), customPropTypes.disallow(['percent']), - PropTypes.oneOfType([ - PropTypes.number, - PropTypes.string, - ]), + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), ]), /** For use with total. Together, these will calculate the percent. Mutually excludes percent. */ value: customPropTypes.every([ customPropTypes.disallow(['percent']), - PropTypes.oneOfType([ - PropTypes.number, - PropTypes.string, - ]), + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), ]), /** A progress bar can show a warning state. */ @@ -115,7 +103,7 @@ class Progress extends Component { const { percent, total, value } = this.props if (!_.isUndefined(percent)) return percent - if (!_.isUndefined(total) && !_.isUndefined(value)) return (value / total) * 100 + if (!_.isUndefined(total) && !_.isUndefined(value)) return value / total * 100 } computeValueText = (percent) => { @@ -129,7 +117,7 @@ class Progress extends Component { getPercent = () => { const { precision, progress, total, value } = this.props const percent = _.clamp(this.calculatePercent(), 0, 100) - if (!_.isUndefined(total) && !_.isUndefined(value) && progress === 'value') return (value / total) * 100 + if (!_.isUndefined(total) && !_.isUndefined(value) && progress === 'value') { return value / total * 100 } if (progress === 'value') return value if (_.isUndefined(precision)) return percent return _.round(percent, precision) @@ -146,18 +134,17 @@ class Progress extends Component { if (!childrenUtils.isNil(children)) return
{children}
if (!childrenUtils.isNil(content)) return
{content}
- return createHTMLDivision(label, { defaultProps: { className: 'label' } }) + return createHTMLDivision(label, { + autoGenerateKey: false, + defaultProps: { className: 'label' }, + }) } renderProgress = (percent) => { const { precision, progress } = this.props if (!progress && _.isUndefined(precision)) return - return ( -
- {this.computeValueText(percent)} -
- ) + return
{this.computeValueText(percent)}
} render() { diff --git a/src/modules/Search/Search.js b/src/modules/Search/Search.js index fc6747940d..b98b93d407 100644 --- a/src/modules/Search/Search.js +++ b/src/modules/Search/Search.js @@ -249,12 +249,20 @@ export default class Search extends Component { debug('search opened') this.open() eventStack.sub('click', this.closeOnDocumentClick) - eventStack.sub('keydown', [this.closeOnEscape, this.moveSelectionOnKeyDown, this.selectItemOnEnter]) + eventStack.sub('keydown', [ + this.closeOnEscape, + this.moveSelectionOnKeyDown, + this.selectItemOnEnter, + ]) } else if (prevState.open && !this.state.open) { debug('search closed') this.close() eventStack.unsub('click', this.closeOnDocumentClick) - eventStack.unsub('keydown', [this.closeOnEscape, this.moveSelectionOnKeyDown, this.selectItemOnEnter]) + eventStack.unsub('keydown', [ + this.closeOnEscape, + this.moveSelectionOnKeyDown, + this.selectItemOnEnter, + ]) } } @@ -262,7 +270,11 @@ export default class Search extends Component { debug('componentWillUnmount()') eventStack.unsub('click', this.closeOnDocumentClick) - eventStack.unsub('keydown', [this.closeOnEscape, this.moveSelectionOnKeyDown, this.selectItemOnEnter]) + eventStack.unsub('keydown', [ + this.closeOnEscape, + this.moveSelectionOnKeyDown, + this.selectItemOnEnter, + ]) } // ---------------------------------------- @@ -414,7 +426,9 @@ export default class Search extends Component { getFlattenedResults = () => { const { category, results } = this.props - return !category ? results : _.reduce(results, (memo, categoryData) => memo.concat(categoryData.results), []) + return !category + ? results + : _.reduce(results, (memo, categoryData) => memo.concat(categoryData.results), []) } getSelectedResult = (index = this.state.selectedIndex) => { @@ -506,6 +520,7 @@ export default class Search extends Component { const { value } = this.state return Input.create(input, { + autoGenerateKey: false, defaultProps: { ...rest, icon, diff --git a/src/modules/Search/SearchResult.js b/src/modules/Search/SearchResult.js index 97b9aaf5ca..133f6cedcf 100644 --- a/src/modules/Search/SearchResult.js +++ b/src/modules/Search/SearchResult.js @@ -19,7 +19,11 @@ import { // Note: To avoid requiring a wrapping div, we return an array here so to // prevent rendering issues each node needs a unique key. const defaultRenderer = ({ image, price, title, description }) => [ - image &&
{createHTMLImage(image)}
, + image && ( +
+ {createHTMLImage(image, { autoGenerateKey: false })} +
+ ),
{price &&
{price}
} {title &&
{title}
} @@ -45,10 +49,7 @@ export default class SearchResult extends Component { description: PropTypes.string, /** A unique identifier. */ - id: PropTypes.oneOfType([ - PropTypes.number, - PropTypes.string, - ]), + id: PropTypes.oneOfType([PropTypes.number, PropTypes.string]), /** Add an image to the item. */ image: PropTypes.string, @@ -93,17 +94,9 @@ export default class SearchResult extends Component { } render() { - const { - active, - className, - renderer, - } = this.props - - const classes = cx( - useKeyOnly(active, 'active'), - 'result', - className, - ) + const { active, className, renderer } = this.props + + const classes = cx(useKeyOnly(active, 'active'), 'result', className) const rest = getUnhandledProps(SearchResult, this.props) const ElementType = getElementType(SearchResult, this.props) diff --git a/src/modules/Tab/Tab.js b/src/modules/Tab/Tab.js index ee67a006ca..3834f7d5dd 100644 --- a/src/modules/Tab/Tab.js +++ b/src/modules/Tab/Tab.js @@ -116,6 +116,7 @@ class Tab extends Component { } return Menu.create(menu, { + autoGenerateKey: false, overrideProps: { items: _.map(panes, 'menuItem'), onItemClick: this.handleItemClick, @@ -132,13 +133,18 @@ class Tab extends Component { return ( - {position === 'left' && GridColumn.create({ width: tabWidth, children: menu })} - {GridColumn.create({ - width: paneWidth, - children: this.renderItems(), - stretched: true, - })} - {position === 'right' && GridColumn.create({ width: tabWidth, children: menu })} + {position === 'left' && + GridColumn.create({ width: tabWidth, children: menu }, { autoGenerateKey: false })} + {GridColumn.create( + { + width: paneWidth, + children: this.renderItems(), + stretched: true, + }, + { autoGenerateKey: false }, + )} + {position === 'right' && + GridColumn.create({ width: tabWidth, children: menu }, { autoGenerateKey: false })} ) } diff --git a/src/views/Card/Card.js b/src/views/Card/Card.js index a36012bf3a..1e172888ba 100644 --- a/src/views/Card/Card.js +++ b/src/views/Card/Card.js @@ -130,15 +130,23 @@ export default class Card extends Component { }) if (!childrenUtils.isNil(children)) { - return {children} + return ( + + {children} + + ) } if (!childrenUtils.isNil(content)) { - return {content} + return ( + + {content} + + ) } return ( - {Image.create(image)} + {Image.create(image, { autoGenerateKey: false })} {(description || header || meta) && ( )} diff --git a/src/views/Card/CardContent.js b/src/views/Card/CardContent.js index e55c19b6f6..0b82955491 100644 --- a/src/views/Card/CardContent.js +++ b/src/views/Card/CardContent.js @@ -47,9 +47,9 @@ function CardContent(props) { return ( - {createShorthand(CardHeader, val => ({ content: val }), header)} - {createShorthand(CardMeta, val => ({ content: val }), meta)} - {createShorthand(CardDescription, val => ({ content: val }), description)} + {createShorthand(CardHeader, val => ({ content: val }), header, { autoGenerateKey: false })} + {createShorthand(CardMeta, val => ({ content: val }), meta, { autoGenerateKey: false })} + {createShorthand(CardDescription, val => ({ content: val }), description, { autoGenerateKey: false })} ) } diff --git a/src/views/Comment/CommentAvatar.js b/src/views/Comment/CommentAvatar.js index f929f7d9b3..c58f2de5b5 100644 --- a/src/views/Comment/CommentAvatar.js +++ b/src/views/Comment/CommentAvatar.js @@ -19,7 +19,11 @@ function CommentAvatar(props) { const rest = getUnhandledProps(CommentAvatar, props) const ElementType = getElementType(CommentAvatar, props) - return {createHTMLImage(src)} + return ( + + {createHTMLImage(src, { autoGenerateKey: false })} + + ) } CommentAvatar._meta = { diff --git a/src/views/Feed/FeedContent.js b/src/views/Feed/FeedContent.js index 489d933d48..298fb97c39 100644 --- a/src/views/Feed/FeedContent.js +++ b/src/views/Feed/FeedContent.js @@ -37,12 +37,12 @@ function FeedContent(props) { return ( - {createShorthand(FeedDate, val => ({ content: val }), date)} - {createShorthand(FeedSummary, val => ({ content: val }), summary)} + {createShorthand(FeedDate, val => ({ content: val }), date, { autoGenerateKey: false })} + {createShorthand(FeedSummary, val => ({ content: val }), summary, { autoGenerateKey: false })} {content} - {createShorthand(FeedExtra, val => ({ text: true, content: val }), extraText)} - {createShorthand(FeedExtra, val => ({ images: val }), extraImages)} - {createShorthand(FeedMeta, val => ({ content: val }), meta)} + {createShorthand(FeedExtra, val => ({ text: true, content: val }), extraText, { autoGenerateKey: false })} + {createShorthand(FeedExtra, val => ({ images: val }), extraImages, { autoGenerateKey: false })} + {createShorthand(FeedMeta, val => ({ content: val }), meta, { autoGenerateKey: false })} ) } diff --git a/src/views/Feed/FeedEvent.js b/src/views/Feed/FeedEvent.js index 91b570d766..7b6bac81da 100644 --- a/src/views/Feed/FeedEvent.js +++ b/src/views/Feed/FeedEvent.js @@ -38,8 +38,8 @@ function FeedEvent(props) { return ( - {createShorthand(FeedLabel, val => ({ icon: val }), icon)} - {createShorthand(FeedLabel, val => ({ image: val }), image)} + {createShorthand(FeedLabel, val => ({ icon: val }), icon, { autoGenerateKey: false })} + {createShorthand(FeedLabel, val => ({ image: val }), image, { autoGenerateKey: false })} {hasContentProp && } {children} diff --git a/src/views/Feed/FeedLabel.js b/src/views/Feed/FeedLabel.js index 241f62ba1d..b3a5e590bf 100644 --- a/src/views/Feed/FeedLabel.js +++ b/src/views/Feed/FeedLabel.js @@ -35,7 +35,7 @@ function FeedLabel(props) { return ( {content} - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {createHTMLImage(image)} ) diff --git a/src/views/Feed/FeedLike.js b/src/views/Feed/FeedLike.js index 684ae25754..44d187fb5b 100644 --- a/src/views/Feed/FeedLike.js +++ b/src/views/Feed/FeedLike.js @@ -32,7 +32,7 @@ function FeedLike(props) { return ( - {Icon.create(icon)} + {Icon.create(icon, { autoGenerateKey: false })} {content} ) diff --git a/src/views/Feed/FeedMeta.js b/src/views/Feed/FeedMeta.js index 9382565a9f..f14733ddb7 100644 --- a/src/views/Feed/FeedMeta.js +++ b/src/views/Feed/FeedMeta.js @@ -33,7 +33,7 @@ function FeedMeta(props) { return ( - {createShorthand(FeedLike, val => ({ content: val }), like)} + {createShorthand(FeedLike, val => ({ content: val }), like, { autoGenerateKey: false })} {content} ) diff --git a/src/views/Feed/FeedSummary.js b/src/views/Feed/FeedSummary.js index 1632e5433f..11e74cb819 100644 --- a/src/views/Feed/FeedSummary.js +++ b/src/views/Feed/FeedSummary.js @@ -35,9 +35,9 @@ function FeedSummary(props) { return ( - {createShorthand(FeedUser, val => ({ content: val }), user)} + {createShorthand(FeedUser, val => ({ content: val }), user, { autoGenerateKey: false })} {content} - {createShorthand(FeedDate, val => ({ content: val }), date)} + {createShorthand(FeedDate, val => ({ content: val }), date, { autoGenerateKey: false })} ) } diff --git a/src/views/Item/Item.js b/src/views/Item/Item.js index e6f4635113..3579b13227 100644 --- a/src/views/Item/Item.js +++ b/src/views/Item/Item.js @@ -42,7 +42,7 @@ function Item(props) { return ( - {ItemImage.create(image)} + {ItemImage.create(image, { autoGenerateKey: false })} - {ItemHeader.create(header)} - {ItemMeta.create(meta)} - {ItemDescription.create(description)} - {ItemExtra.create(extra)} + {ItemHeader.create(header, { autoGenerateKey: false })} + {ItemMeta.create(meta, { autoGenerateKey: false })} + {ItemDescription.create(description, { autoGenerateKey: false })} + {ItemExtra.create(extra, { autoGenerateKey: false })} {content} ) diff --git a/src/views/Statistic/Statistic.js b/src/views/Statistic/Statistic.js index fa55bc94b5..ea6d082a20 100644 --- a/src/views/Statistic/Statistic.js +++ b/src/views/Statistic/Statistic.js @@ -56,8 +56,9 @@ function Statistic(props) { {StatisticValue.create(value, { defaultProps: { text }, + autoGenerateKey: false, })} - {StatisticLabel.create(label)} + {StatisticLabel.create(label, { autoGenerateKey: false })} ) } diff --git a/test/specs/commonTests/implementsShorthandProp.js b/test/specs/commonTests/implementsShorthandProp.js index 28eac09618..480b31e0ac 100644 --- a/test/specs/commonTests/implementsShorthandProp.js +++ b/test/specs/commonTests/implementsShorthandProp.js @@ -20,6 +20,8 @@ const shorthandComponentName = (ShorthandComponent) => { * @param {string|function} options.ShorthandComponent The component that should be rendered from the shorthand value. * @param {boolean} [options.alwaysPresent] Whether or not the shorthand exists by default. * @param {boolean} [options.assertExactMatch] Selects an assertion method, `contain` will be used if true. + * @param {boolean} [options.autoGenerateKey=false] Whether or not automatic key generation is + * allowed for the shorthand component. * @param {function} options.mapValueToProps A function that maps a primitive value to the Component props. * @param {Object} [options.requiredProps={}] Props required to render the component. * @param {Object} [options.shorthandDefaultProps] Default props for the shorthand component. @@ -29,6 +31,7 @@ export default (Component, options = {}) => { const { alwaysPresent, assertExactMatch = true, + autoGenerateKey = false, mapValueToProps, propKey, ShorthandComponent, @@ -47,13 +50,21 @@ export default (Component, options = {}) => { const name = shorthandComponentName(ShorthandComponent) const assertValidShorthand = (value) => { - const shorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, { + const expectedShorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, { defaultProps: shorthandDefaultProps, overrideProps: shorthandOverrideProps, + autoGenerateKey, }) const element = createElement(Component, { ...requiredProps, [propKey]: value }) + const wrapper = shallow(element) - shallow(element).should[assertMethod](shorthandElement) + wrapper.should[assertMethod](expectedShorthandElement) + + // Enzyme's .key() method is not consistent with React for elements with + // no key (`undefined` vs `null`), so use the underlying element instead + // Will fail if more than one element of this type is found + const shorthandElement = wrapper.find(ShorthandComponent).getElement() + expect(shorthandElement.key).to.equal(expectedShorthandElement.key, "key doesn't match") } if (alwaysPresent || (Component.defaultProps && Component.defaultProps[propKey])) { diff --git a/test/specs/elements/Label/LabelDetail-test.js b/test/specs/elements/Label/LabelDetail-test.js index b6bd6778ae..369cd6ad1d 100644 --- a/test/specs/elements/Label/LabelDetail-test.js +++ b/test/specs/elements/Label/LabelDetail-test.js @@ -3,5 +3,6 @@ import LabelDetail from 'src/elements/Label/LabelDetail' describe('LabelDetail', () => { common.isConformant(LabelDetail) + common.implementsCreateMethod(LabelDetail) common.rendersChildren(LabelDetail) }) diff --git a/test/specs/lib/factories-test.js b/test/specs/lib/factories-test.js index 7c05f0b2f5..07b3cec5b8 100644 --- a/test/specs/lib/factories-test.js +++ b/test/specs/lib/factories-test.js @@ -16,8 +16,9 @@ const getShorthand = ({ defaultProps, mapValueToProps = () => ({}), overrideProps, + autoGenerateKey, value, -}) => createShorthand(Component, mapValueToProps, value, { defaultProps, overrideProps }) +}) => createShorthand(Component, mapValueToProps, value, { defaultProps, overrideProps, autoGenerateKey }) // ---------------------------------------- // Common tests @@ -209,6 +210,30 @@ describe('factories', () => { getShorthand({ value: { key: '' } }).should.have.property('key', '') }) }) + + describe('when value is a string', () => { + it('is generated from the value', () => { + getShorthand({ value: 'foo' }) + .should.have.property('key', 'foo') + }) + + it('is not generated if autoGenerateKey is false', () => { + getShorthand({ value: 'foo', autoGenerateKey: false }) + .should.have.property('key', null) + }) + }) + + describe('when value is a number', () => { + it('is generated from the value', () => { + getShorthand({ value: 123 }) + .should.have.property('key', '123') + }) + + it('is not generated if autoGenerateKey is false', () => { + getShorthand({ value: 123, autoGenerateKey: false }) + .should.have.property('key', null) + }) + }) }) describe('childKey', () => {