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

docs(Portal): fix controlled Portal usage #3420

Merged
merged 3 commits into from
Feb 13, 2019
Merged

docs(Portal): fix controlled Portal usage #3420

merged 3 commits into from
Feb 13, 2019

Conversation

Fabianopb
Copy link
Member

As discussed in #3408 the usage of Portal as a controlled component was wrong in the docs creating a false positive bug. The proper way puts the control inside the Portal's trigger:

image

(Closes #3408)

@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #3420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3420   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files         172      172           
  Lines        2838     2838           
=======================================
  Hits         2836     2836           
  Misses          2        2

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 2b8a7eb...da50215. Read the comment docs.

@layershifter
Copy link
Member

Actually, the issue comes from EventStack that incorrectly works with browser frames (click on button should not be fired actually).

There is an experiment with new approach: https://codesandbox.io/s/9yw3vq6774 I need to spend more time on it to test other scenarios and perform cleanup.

I will update the example later today.

@Fabianopb
Copy link
Member Author

So would that mean that the fix should be done in EventStack in the end?

@layershifter
Copy link
Member

So would that mean that the fix should be done in EventStack in the end?

@Fabianopb Actually, yes. But, I'm not ready to ship a correct fix for right now. I need to resolve the issue correctly and forever 👍

@layershifter layershifter changed the title Fix controlled Portal usage (#3408). docs(Portal): fix controlled Portal usage Feb 12, 2019
@layershifter
Copy link
Member

@Fabianopb I've updated an example, now it works more correctly. Please let me know if it is fine 🚶

@Fabianopb
Copy link
Member Author

Now the external button is solely used to open the Portal, which can be closed via the internal button or by clicking outside. Sounds fair too!

@layershifter layershifter merged commit 8365c90 into Semantic-Org:master Feb 13, 2019
@levithomason
Copy link
Member

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

4 participants