Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: s/validate/primary/ & s/cancel/secondary/ #78

Merged
merged 1 commit into from
Mar 15, 2017
Merged

feat: s/validate/primary/ & s/cancel/secondary/ #78

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 80 additions & 39 deletions react/Modal/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,57 +3,98 @@ import classNames from 'classnames'

import styles from './index.styl'

const Modal = ({
title, description,
cancelType, cancelText, cancelAction,
validateType, validateText, validateAction, children
}) => (
<div className={styles['coz-modal-container']}>
<div className={styles['coz-overlay']}>
<div className={styles['coz-modal']}>
<h2 className={styles['coz-modal-title']}>{title}</h2>
<button
className={classNames('coz-btn', 'coz-btn--close', styles['coz-btn-modal-close'])}
onClick={cancelAction}
>
<span className='coz-hidden'>{cancelText}</span>
</button>
{ description && <div className={classNames(styles['coz-modal-content'], styles['coz-description'])}>
{description}
</div>
const ModalTitle = ({ title }) =>
(
<h2 className={styles['coz-modal-title']}>{title}</h2>
)

const ModalCross = ({ withCross, secondaryAction, secondaryText }) =>
withCross &&
(
<button
className={classNames('coz-btn', 'coz-btn--close', styles['coz-btn-modal-close'])}
onClick={secondaryAction}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope we'll never have to bind different actions to the cross and the secondary button. But I think this is good as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need it later... We could change it later.

>
<span className='coz-hidden'>{secondaryText}</span>
</button>
)

const ModalDescription = ({ description }) =>
description &&
(
<div className={classNames(styles['coz-modal-content'], styles['coz-description'])}>
{description}
</div>
)

const ModalButtons = ({ secondaryText, secondaryAction, secondaryType, primaryText, primaryAction, primaryType }) => {
const displayPrimary = primaryText && primaryAction
const displaySecondary = secondaryText && secondaryAction
return (displaySecondary || displayPrimary) &&
(
<div className={classNames(styles['coz-modal-content'], styles['coz-modal-buttons'])}>
{ displaySecondary &&
<button className={classNames('coz-btn', 'coz-btn--' + secondaryType)} onClick={secondaryAction}>
{secondaryText}
</button>
}
{ children }
{ ((cancelText && cancelAction) || (validateText && validateAction)) &&
<div className={classNames(styles['coz-modal-content'], styles['coz-modal-buttons'])}>
{ (cancelText && cancelAction) && <button className={classNames('coz-btn', 'coz-btn--' + cancelType)} onClick={cancelAction}>
{cancelText}
</button>
}
{ (validateText && validateAction) && <button className={classNames('coz-btn', 'coz-btn--' + validateType)} onClick={validateAction}>
{validateText}
</button>
}
</div>
{ displayPrimary &&
<button className={classNames('coz-btn', 'coz-btn--' + primaryType)} onClick={primaryAction}>
{primaryText}
</button>
}
</div>
)
}

const Modal = (props) => {
props.primaryType = props.primaryType || props.validateType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use http://babeljs.io/docs/plugins/transform-es2015-parameters/ instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not use destructuring here because I think:

const Modal = ({ title, description, primaryType, primaryText, primaryAction, secondaryType, secondaryText, secondaryAction, validateType, validateText, validateAction, cancelType, cancelText, cancelAction, withCross }) => { /* etc. */ }

was less clean than

const Modal = (props) => { /* etc. */ }

mostly when I decomposed the massive Modal component into smaller one by passing them ...props instead of the props they need explicitly. Especially with a component that declares its propTypes.

But I may be wrong.

What do you think about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, missed the fat arrow. Ok for me.

props.primaryText = props.primaryText || props.validateText
props.primaryAction = props.primaryAction || props.validateAction
props.secondaryType = props.secondaryType || props.cancelType
props.secondaryText = props.secondaryText || props.cancelText
props.secondaryAction = props.secondaryAction || props.cancelAction
return (
<div className={styles['coz-modal-container']}>
<div className={styles['coz-overlay']}>
<div className={styles['coz-modal']}>
<ModalTitle {...props} />
<ModalCross {...props} />
<ModalDescription {...props} />
{ props.children }
<ModalButtons {...props} />
</div>
</div>
</div>
</div>
)
)
}

function deprecated(arg) {
console.warn(`[Modal] ${arg} is deprecated, please upgrade your Modal component.`)
}

Modal.propTypes = {
title: React.PropTypes.string.isRequired,
description: React.PropTypes.node,
cancelType: React.PropTypes.string,
cancelText: React.PropTypes.string,
cancelAction: React.PropTypes.func,
validateType: React.PropTypes.string,
validateText: React.PropTypes.string,
validateAction: React.PropTypes.func
secondaryType: React.PropTypes.string,
secondaryText: React.PropTypes.string,
secondaryAction: React.PropTypes.func,
primaryType: React.PropTypes.string,
primaryText: React.PropTypes.string,
primaryAction: React.PropTypes.func,
cancelType: deprecated('cancelType') || React.PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not React.ProTypes.string || deprecated('cancelType') ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to evaluate deprecated() (to display the deprecated warning in the console) but do not want to change the value of the cancelType parameter.

And as you can see in the two last examples in the mdn (logical OR) page:

o8 = '' || false // returns false
o9 = false || '' // returns ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@enguerran : ok, deprecated is a function that first warn the developpeur that the result will be false soon, and later is really deprecated ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is right. they are temporary instructions that warn the developer. As soon as others apps stop using those props, we remove these declarations (and the ones in the Modal function).

cancelText: deprecated('cancelText') || React.PropTypes.string,
cancelAction: deprecated('cancelAction') || React.PropTypes.func,
validateType: deprecated('validateType') || React.PropTypes.string,
validateText: deprecated('validateText') || React.PropTypes.string,
validateAction: deprecated('validateAction') || React.PropTypes.func,
withCross: React.PropTypes.bool
}

Modal.defaultProps = {
cancelType: 'secondary',
validateType: 'primary'
primaryType: 'secondary',
withCross: true
}

export default Modal