Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Popup): restore ability to render popup from React element #592

Merged
merged 5 commits into from
Dec 11, 2018

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Dec 11, 2018

TODO

  • update changelog

With recent changes being introduced to shorthand factories (specifically, because of the change that now dictates to pass React Element shorthand value 'as is'), the following way of defining Popup has become broken:

<Popup content={<p>This is the content that will be rendered 'as is'</p>} ... />

The way it was broken is that now the content of the <p> element is, essentially, not visible (however, while still rendered in the DOM tree), because positioning styles are not applied.


This corresponds to the following example on the docs page:

image


This PR restores this functionality.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

This issue shows that our idea from #496 may be not so good.

The factories change is not released yet, so I will block this PR for additional discussion on sync.

Co-Authored-By: kuzhelov <deckster@mail.ru>
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Unblocked, let's go.

@kuzhelov
Copy link
Contributor Author

@layershifter

As we've agreed with @layershifter while having discussion around that, moving with this now but leaving this topic for future discussion around shorthand factories, as this change might serve as a warning sign for the approach we've taken, especially if this pattern

isValidElement() ? clone() : create()   // © @layershifter :)

will be repeated for other components in many places.

@kuzhelov kuzhelov merged commit 7b7603a into master Dec 11, 2018
@kuzhelov kuzhelov deleted the fix/popup-rendered-from-react-element branch December 11, 2018 13:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants