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.

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

breaking(Modal): update shorthand functionality #1599

Merged
merged 9 commits into from
Aug 27, 2017

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Apr 19, 2017

Breaking Changes

Previously, shorthand Modal actions required passing button props objects with triggerClose: true on buttons that should close the Modal.

Now, all shorthand Modal actions buttons close the Modal automatically. The triggerClose property has been removed.

Before

<Modal
  header='Reminder!'
  content='Call Benjamin.'
  actions={[
    { key: 'snooze', content: 'Snooze', triggerClose: true },
    { key: 'done', content: 'Done', positive: true, triggerClose: true },
  ]}
/>

After

<Modal
  header='Reminder!'
  content='Call Benjamin.'
  actions={[
    'Snooze',
    { key: 'done', content: 'Done', positive: true },
  ]}
/>

Blocking #1542

This PR:

  • removes the triggerClose shorthand Modal actions property
  • makes all shorthand actions close the Modal
  • fix prop types / typings

@levithomason levithomason changed the title wip refactor(Modal): update shorthand functionality Apr 19, 2017
@levithomason levithomason changed the title refactor(Modal): update shorthand functionality WIP: refactor(Modal): update shorthand functionality Apr 19, 2017
/** Elements to render as Modal action buttons. */
actions: PropTypes.arrayOf(customPropTypes.itemShorthand),
/** Shorthand for Modal.Actions. Typically an array of button shorthand. */
actions: customPropTypes.itemShorthand,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this change. I think that we should use there customPropTypes.collectionShorthand.

PropTypes.bool,
]),
/** Shorthand for the close icon. Closes the modal on click. */
closeIcon: customPropTypes.itemShorthand,
Copy link
Member

Choose a reason for hiding this comment

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

customPropTypes.itemShorthand disallows usage of this prop with children. If we will make this change, we should also make changes in the render method.

@codecov-io
Copy link

codecov-io commented May 23, 2017

Codecov Report

Merging #1599 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1599   +/-   ##
======================================
  Coverage    99.8%   99.8%           
======================================
  Files         148     148           
  Lines        2590    2590           
======================================
  Hits         2585    2585           
  Misses          5       5
Impacted Files Coverage Δ
src/modules/Modal/ModalActions.js 100% <ø> (ø) ⬆️
src/modules/Modal/Modal.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c4972...169c2cf. Read the comment docs.

@layershifter
Copy link
Member

layershifter commented May 23, 2017

I've pushed some changes there:

  • updated to latest master;
  • reverted change of type in typings;
  • changed actions to collection shorthand;
  • added onActionClick prop;
  • fixed broken tests.

TODO

  • make a decision about closeIcon prop, it will be breaking change if we'll left it as customPropTypes.itemShorthand;
  • solve warnings with implementsCreateMethod test on ModalActions, I think we to update it with new option, that will skip non array inputs.

@levithomason I'll be glad to hear your thoughts.

@a-x-
Copy link

a-x- commented Aug 21, 2017

ping

@levithomason levithomason force-pushed the refactor/modal-shorthand branch from 6e04023 to 36b7136 Compare August 27, 2017 17:24
@levithomason levithomason changed the title WIP: refactor(Modal): update shorthand functionality breaking(Modal): update shorthand functionality Aug 27, 2017
@levithomason
Copy link
Member Author

I've rebased to the latest master and resolved all issues:

closeIcon - Leaving this as is for minimal churn, allowing with children.

Updated actions propType to itemShorthand since it is not an array of buttons but the props to Modal.Actions. You can pass an array of buttons because mapValueToProps for ModalActions maps the array to the Modal.Actions actions prop, which is an array of buttons.

@levithomason levithomason merged commit 2e1ddee into master Aug 27, 2017
@levithomason levithomason deleted the refactor/modal-shorthand branch August 27, 2017 21:55
@levithomason
Copy link
Member Author

Released in semantic-ui-react@0.72.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants