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

fix(Modal|Portal|Popup): use proposed value for open in onOpen & onClose callbacks #4030

Merged
merged 3 commits into from
Sep 17, 2020
Merged

fix(Modal|Portal|Popup): use proposed value for open in onOpen & onClose callbacks #4030

merged 3 commits into from
Sep 17, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Aug 11, 2020 •
edited

Fixes #1280.

⚠️ BREAKING CHANGES

This PR contains breaking changes in API.

It updates the behavior of onOpen & onClose callbacks in Modal, Popup & Portal components.

The open prop in the data param to callbacks will be the proposed open value, not the current value as was before. This way, it can be used in an onOpen => open loop for a controlled component. Identical to the onChange => value loop for an input, checkbox, etc. The value is the would-be-value should the update take effect, not the current controlled value.

Before

<>
  <Portal
    onOpen={(e, data) => console.log(data.open /* open = false */)}
    onClose={(e, data) => console.log(data.open /* open = true */)}
  />
  <Modal
    onOpen={(e, data) => console.log(data.open /* open = false */)}
    onClose={(e, data) => console.log(data.open /* open = true */)}
  />
</>
<Portal
  onOpen={(e, data) => setOpen(true)}
  onClose={(e, data) => setOpen(false)}
/>

After

<>
  <Portal
    onOpen={(e, data) => console.log(data.open /* open = true */)}
    onClose={(e, data) => console.log(data.open /* open = false */)}
  />
  <Modal
    onOpen={(e, data) => console.log(data.open /* open = true */)}
    onClose={(e, data) => console.log(data.open /* open = false */)}
  />
</>
{/* This usage is unblocked now */}
<Portal
  onOpen={(e, data) => setOpen(data.open)}
  onClose={(e, data) => setOpen(data.open)}
/>

@github-actions
Copy link

github-actions bot commented Aug 11, 2020 •
edited

size-limit report

Path Size
bundle-size/dist/Button.size.js 66.48 KB (+0.04% 🔺)
bundle-size/dist/Icon.size.js 30.65 KB (0%)
bundle-size/dist/Image.size.js 61.72 KB (+0.04% 🔺)
bundle-size/dist/Modal.size.js 76.79 KB (+0.09% 🔺)
bundle-size/dist/Portal.size.js 47.8 KB (+0.05% 🔺)

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2020 •
edited

Codecov Report

Merging #4030 into next-v2 will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           next-v2    #4030      +/-   ##
===========================================
- Coverage    99.84%   99.84%   -0.01%     
===========================================
  Files          183      183              
  Lines         3280     3278       -2     
===========================================
- Hits          3275     3273       -2     
  Misses           5        5
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100.00% <100.00%> (ø)
src/modules/Modal/Modal.js 100.00% <100.00%> (ø)
src/modules/Popup/Popup.js 98.75% <100.00%> (ø)

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 40903c2...a651bea. Read the comment docs.

@layershifter layershifter changed the base branch from master to next-v2 September 17, 2020 15:28
@layershifter layershifter merged commit 5e0a366 into next-v2 Sep 17, 2020
@layershifter layershifter deleted the breaking/portal-changes branch September 17, 2020 15:28
layershifter added a commit that referenced this pull request Sep 29, 2020
@layershifter
fix(Modal|Portal|Popup): use proposed value for open in onOpen & …
8f92ac8 
…`onClose` callbacks (#4030)

fix(Modal|Portal|Popup): use proposed value for `open` in `onOpen` & `onClose` callbacks
@layershifter
Copy link
Member Author

Released in semantic-ui-react@2.0.0 🚢

@jrcastro2 jrcastro2 mentioned this pull request Jan 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR: Breaking Change 💥
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC: Portal improvements
2 participants
@layershifter @codecov-commenter