style(Confirm): refactor, update typings and propTypes usage #1282
Conversation
Codecov Report@@ Coverage Diff @@
## master #1282 +/- ##
==========================================
- Coverage 99.74% 95.92% -3.83%
==========================================
Files 140 883 +743
Lines 2394 4929 +2535
==========================================
+ Hits 2388 4728 +2340
- Misses 6 201 +195
Continue to review full report at Codecov.
|
layershifter
force-pushed
the
style/confirm
branch
from
b90d5ec to
1986efb
Compare
src/addons/Confirm/Confirm.js
Outdated
| type: META.TYPES.ADDON, | ||
| } | ||
| /** The OK button text. */ | ||
| confirmButton: PropTypes.string, |
There was a problem hiding this comment.
I'm also thinking about shorthand there too.
-confirmButton: PropTypes.string,
+confirm: customPropTypes.itemShorthand,
There was a problem hiding this comment.
That would be great if you'd like to implement it. 👍
There was a problem hiding this comment.
Okay, I'll make it today 😄
layershifter
force-pushed
the
style/confirm
branch
from
1986efb to
7a138f0
Compare
src/addons/Confirm/Confirm.js
Outdated
| type: META.TYPES.ADDON, | ||
| } | ||
| /** The OK button text. */ | ||
| confirmButton: PropTypes.string, |
There was a problem hiding this comment.
That would be great if you'd like to implement it. 👍
src/addons/Confirm/Confirm.js
Outdated
| return ( | ||
| <Modal {...rest} {...openProp} size='small' onClose={this.handleCancel}> | ||
| {createShorthand(Modal.Header, val => ({ children: val }), header)} | ||
| {createShorthand(Modal.Content, val => ({ children: val }), content)} |
There was a problem hiding this comment.
I'm slowing trying to remove use of createShorthand in favor of adding create() methods to the component when needed. We do plan to have shorthand for the Modal, which we currently don't, so having the create() methods for ModalHeader and ModalContent would be helpful.
Let's remove createShorthand and add the create() method for these components instead.
src/addons/Confirm/Confirm.js
Outdated
| /** The OK button text */ | ||
| confirmButton: PropTypes.string, | ||
| /** | ||
| * Called when the Cancel button is clicked. |
There was a problem hiding this comment.
Let's correct this, "Called when the Modal is closed without clicking confirm." Since, it is called on Escape and click outside as well.
src/addons/Confirm/index.d.ts
Outdated
| header?: any; | ||
|
|
||
| /** | ||
| * Called when the Cancel button is clicked. |
There was a problem hiding this comment.
See my comment for the onClick propType.
Thanks, this is how it should've been written initially. I don't think we'll need to make this one breaking since the previous behavior was a bug and this fixes it. Also, the callback did not advertise a second |
layershifter
force-pushed
the
style/confirm
branch
from
7a138f0 to
44bf621
Compare
|
@levithomason I've updated PR, take a look 😄 |
|
Released in |
This PR is part of work for removing propTypes in production bundle (#524, #731).
Also, cleanups and updates typings for #1072.
Fixes #1290.
Warning. This PR updates behavior of
onCancelandonConfirm, now it handled insideConfirmcomponent.@levithomason I think we need to mark this change as breaking. Before
datawasButton's props while after this PR it willConfirms props.