-
Notifications
You must be signed in to change notification settings - Fork 37
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: s/validate/primary/ & s/cancel/secondary/ #78
Conversation
Needed for cozy/cozy-drive#172 |
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.
Very cool, much approve 🐶
( | ||
<button | ||
className={classNames('coz-btn', 'coz-btn--close', styles['coz-btn-modal-close'])} | ||
onClick={secondaryAction} |
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 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.
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 we need it later... We could change it later.
} | ||
|
||
const Modal = (props) => { | ||
props.primaryType = props.primaryType || props.validateType |
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.
❤️
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.
Why not use http://babeljs.io/docs/plugins/transform-es2015-parameters/ instead ?
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 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?
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 yes, missed the fat arrow. Ok for me.
primaryType: React.PropTypes.string, | ||
primaryText: React.PropTypes.string, | ||
primaryAction: React.PropTypes.func, | ||
cancelType: deprecated('cancelType') || React.PropTypes.string, |
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.
Why is it not React.ProTypes.string || deprecated('cancelType')
?
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 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 ""
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.
@enguerran : ok, deprecated is a function that first warn the developpeur that the result will be false soon, and later is really deprecated ?
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.
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).
To accept retro-compatibility, I had a
console.warn
for future deprecated props.I also change two things:
withCross
as a bool to control the cross display