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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b450b5
render popup as body children
kuzhelov-ms Oct 2, 2018
f2b71b1
changelog
kuzhelov-ms Oct 3, 2018
876d06c
Merge branch 'master' into fix/popup-accessibility-aspects
kuzhelov Oct 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Fixes
- Fix Attachment `styles` prop typing @levithomason ([#299](https://github.com/stardust-ui/react/pull/299))
- Fix generation of `key` for the `Accordion.Content` @mnajdova ([#305](https://github.com/stardust-ui/react/pull/305))
- Ensure `Popup` is rendered as direct child of `body` element in the DOM tree @kuzhelov ([#302](https://github.com/stardust-ui/react/pull/302))

### Features
- Add focus styles for `Menu.Item` component @Bugaa92 ([#286](https://github.com/stardust-ui/react/pull/286))
Expand Down
9 changes: 7 additions & 2 deletions src/components/Popup/Popup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import * as React from 'react'
import { createPortal } from 'react-dom'
import * as PropTypes from 'prop-types'
import _ from 'lodash'
import { Popper, PopperChildrenProps } from 'react-popper'

import { childrenExist, AutoControlledComponent, IRenderResultConfig } from '../../lib'
import { childrenExist, AutoControlledComponent, IRenderResultConfig, isBrowser } from '../../lib'
import {
ComponentEventHandler,
ItemShorthand,
Expand Down Expand Up @@ -107,6 +108,8 @@ export default class Popup extends AutoControlledComponent<Extendable<IPopupProp

public static autoControlledProps = ['open']

private static isBrowserContext = isBrowser()

protected actionHandlers: AccessibilityActionHandlers = {
toggle: e =>
_.invoke(this.props, 'onOpenChange', e, { ...this.props, ...{ open: !this.props.open } }),
Expand Down Expand Up @@ -138,7 +141,9 @@ export default class Popup extends AutoControlledComponent<Extendable<IPopupProp
...accessibility.keyHandlers.trigger,
})}
</Ref>
{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 •
edited
Loading

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.

</>
)
}
Expand Down