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

fix(Popup): render element as body children #302

Merged
merged 3 commits into from
Oct 3, 2018

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Oct 2, 2018

This PR fixes issue #294 by ensuring that Popup component's DOM tree is rendered as body's child element.

@codecov
Copy link

codecov bot commented Oct 2, 2018

Codecov Report

Merging #302 into master will decrease coverage by 0.13%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
- Coverage   89.63%   89.49%   -0.14%     
==========================================
  Files          62       62              
  Lines        1167     1171       +4     
  Branches      173      152      -21     
==========================================
+ Hits         1046     1048       +2     
- Misses        119      121       +2     
  Partials        2        2
Impacted Files Coverage Δ
src/components/Popup/Popup.tsx 42.59% <50%> (+0.59%) ⬆️

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 6c53400...876d06c. Read the comment docs.

{open && this.renderPopupContent(rtl, accessibility)}
{open &&
Popup.isBrowserContext &&
createPortal(this.renderPopupContent(rtl, accessibility), document.body)}
Copy link
Member

Choose a reason for hiding this comment

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

This is probably more related to #282 but what's the reason to use React Portal directly instead of using stardust Portal component?
The main benefit of using stardust Portal component would be a reuse of onOutsideClick (close popup when user clicks outside the popup/trigger) and similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, also stardust Portal component is going to have Focus Trap Zone, which can be used in Popup too, by just passing prop to enable it.
Otherwise, we would need to duplicate logic with focus trap in Popup component

Copy link
Contributor Author

@kuzhelov kuzhelov Oct 3, 2018

Choose a reason for hiding this comment

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

would rather like to address it by means of separate PR. The reason for that is, primarily, the following: apart from introducing this common functionality, the Portal component introduces quite a few additional conventions, such as render as direct body's children, etc - and there are not intuitive for the client given just the Portal's name and API - probably, it should be renamed first.

So, my intuition is to introduce the functionality that is necessary as a first thing, and after that generalize by making refactoring of Portal component - in a way of making this component itself more generic and properly translate its functionality and application strategies via its name and API (BodyPortal is something that comes to my head right now, but again, would rather like to see a clear picture first). More than that, now it seems that only these two aspects are addressed by Portal - and thus it is quite easy to introduce a dedicated function instead of component for that. One of the pros of this approach is that we will save one component from the Popup's rendered tree.

Please, let me know about your thoughts on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

If these changes are unblocking some other pull request, I am okay with them being merged and then the Portal can be replace with Stardust's Portal. In that way we can make progress on both aspects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have discussed with @sophieH29 - essentially, have agreed on the same plan. Lets wait on @miroslavstastny's comments and merge if there won't be anything critical.

Copy link
Member

Choose a reason for hiding this comment

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

Go for now.

@kuzhelov kuzhelov added ready for merge postponed Item is postponed and will be reconsidered in future and removed 🚀 ready for review labels Oct 3, 2018
@kuzhelov kuzhelov removed the postponed Item is postponed and will be reconsidered in future label Oct 3, 2018
@kuzhelov kuzhelov merged commit fe60933 into master Oct 3, 2018
@layershifter layershifter deleted the fix/popup-accessibility-aspects branch November 9, 2018 09:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants