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

feat(TransitionablePortal): add component #2155

Merged
merged 6 commits into from
Oct 29, 2017

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Oct 2, 2017

Why?

  • Modal requires the page Dimmer
  • Page Dimmer requires transitions, but it uses Portal
  • Portal unmounts components when closes

Solution

We need to simply wrap Portal into the new components that respects Transition's callbacks.

But, we can use Portal?

Portal is already complicated, this will be overhang for it as callback handling is quite complex.

@layershifter
Copy link
Member Author

@levithomason It's ready for initial review.

Naming

I'm not sure that I chose the best name for the component, too many characters as I think.

Behaviour

I have trouble with onClose handling, the try to click multiple on Button in docs and you will undertand troubles :)


I also need to add more tests after initial review.

@codecov-io
Copy link

codecov-io commented Oct 3, 2017

Codecov Report

Merging #2155 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2155      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files         151      152       +1     
  Lines        2621     2651      +30     
==========================================
+ Hits         2614     2644      +30     
  Misses          7        7
Impacted Files Coverage Δ
...ddons/TransitionablePortal/TransitionablePortal.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 4d28298...1b11690. Read the comment docs.

@levithomason
Copy link
Member

OK, thanks. Skimming through PRs/Issues right now trying to get a release out. I will come back to this one as it seems like it needs more attention than I have at the moment.

@levithomason
Copy link
Member

Reviewing and testing...

componentWillReceiveProps({ open }) {
debug('componentWillReceiveProps()', { open })

// Heads up! We apply `open` prop only when it's defined, otherwise it will break autocontrolled Portal
Copy link
Member

Choose a reason for hiding this comment

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

This component does not extend ACC, is this still valid?

Copy link
Member

Choose a reason for hiding this comment

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

Resolved, see my next...


// Heads up! We simply call `onClose` when component is controlled with `open` prop.
// But, when it's autocontrolled we should change the state to opposite to keep the transition
// queue
Copy link
Member

Choose a reason for hiding this comment

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

This component does not extend ACC, is this still valid?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. This is referring to the underlying Portal which extends ACC.

@levithomason
Copy link
Member

My first thought was just to add an unmountOnClose prop to the Portal, however, leaving lots of Portal components mounted to the DOM doesn't sound like a good idea. Also, we do want to unmount them, just not until the Transition is done.

Your solution seems like a sane approach to this problem for now.

Name

It's great. I'd rather have it verbose and clear than short. Perhaps since we have Transition and Portal, we just call it PortalTransition?

Behavior

Ah, this is tricky. Clicking the trigger immediately calls onClose/onOpen, because those callbacks are triggered from the Portal props. However, the PortalTransition is not truly closed until the transition has completed. Therefore, the authoritative callback for "opened" is onOpen from the Portal and the authoritative callback for "closed" is onHide from the Transition.

Changing the example from onClose to onHide resolves the issue. The button state also waits for the Transition before changing. This is correct and prevents entering an invalid mix of states.

Suggest Fix

We probably shouldn't expose the onOpen and onClose callbacks at all. Just use onStart and onHide. These are called at the actual time the state changes.

@layershifter
Copy link
Member Author

@levithomason I've made changes, thanks for your suggestions. Ready for review

@levithomason levithomason merged commit de69da9 into master Oct 29, 2017
@levithomason levithomason deleted the feat/transitionable-portal branch October 29, 2017 15:54
@levithomason
Copy link
Member

Released in semantic-ui-react@0.76.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.

None yet

3 participants