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

fix(Popup): opened popup causes infinite rendering loop #705

Merged
merged 4 commits into from
Jan 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix shorthand prop type @kuzhelov ([#697](https://github.com/stardust-ui/react/pull/697))
- Export `ShorthandRenderer` type @miroslavstastny ([#698](https://github.com/stardust-ui/react/pull/698))
- Temporary revert `pxToRem` changes introduced by [#371](https://github.com/stardust-ui/react/pull/371) @kuzhelov ([#700](https://github.com/stardust-ui/react/pull/700))
- Prevent infinite rendering loop start on `Popup` open @kuzhelov ([#705](https://github.com/stardust-ui/react/pull/705))

### Documentation
- Add ability to edit examples' code in JavaScript and TypeScript @layershifter ([#650](https://github.com/stardust-ui/react/pull/650))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as React from 'react'
import { Button, Popup, Segment } from '@stardust-ui/react'

class AsyncDataLoader extends React.Component<any, any> {
state = {
data: 'loading..',
}

componentDidMount() {
setTimeout(() => {
this.setState({
data: <Segment styles={{ minHeight: '300px' }}>Hello from loaded data!</Segment>,
})
this.props.onLoaded()
}, 1000)
}

render() {
return this.state.data
}
}

const PopupExampleAsync = () => (
<Popup
trigger={<Button icon="expand" content="Click me!" />}
renderContent={updatePosition => ({ content: <AsyncDataLoader onLoaded={updatePosition} /> })}
/>
)

export default PopupExampleAsync
5 changes: 5 additions & 0 deletions docs/src/examples/components/Popup/Usage/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import ExampleSection from 'docs/src/components/ComponentDoc/ExampleSection'

const Usage = () => (
<ExampleSection title="Usage">
<ComponentExample
title="Async popup position update"
description="A popup can be forced to update its position - this comes in handy in async data loading scenarios."
examplePath="components/Popup/Usage/PopupExampleAsync"
/>
<ComponentExample
title="Triggering popup on different actions"
description="A popup can be triggered on click, hover or focus."
Expand Down
15 changes: 10 additions & 5 deletions src/components/Popup/Popup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ export interface PopupProps
*/
position?: Position

/**
* Function to render popup content.
* @param {Function} updatePosition - function to request popup position update.
*/
renderContent?: (updatePosition: Function) => ShorthandValue

/**
* DOM element that should be used as popup's target - instead of 'trigger' element that is used by default.
*/
Expand Down Expand Up @@ -140,6 +146,7 @@ export default class Popup extends AutoControlledComponent<ReactProps<PopupProps
open: PropTypes.bool,
onOpenChange: PropTypes.func,
position: PropTypes.oneOf(POSITIONS),
renderContent: PropTypes.func,
target: PropTypes.any,
trigger: PropTypes.any,
}
Expand Down Expand Up @@ -411,9 +418,11 @@ export default class Popup extends AutoControlledComponent<ReactProps<PopupProps
popupPositionClasses: string,
rtl: boolean,
accessibility: AccessibilityBehavior,
// https://popper.js.org/popper-documentation.html#Popper.scheduleUpdate
{ ref, scheduleUpdate, style: popupPlacementStyles }: PopperChildrenProps,
) => {
const { content } = this.props
const { content: propsContent, renderContent } = this.props
const content = renderContent ? renderContent(scheduleUpdate) : propsContent
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure but, maybe it will be nice if we can have an example with this, especially that we are adding some new prop for the API. Other then that looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, will work on it!


const popupWrapperAttributes = {
...(rtl && { dir: 'rtl' }),
Expand Down Expand Up @@ -443,10 +452,6 @@ export default class Popup extends AutoControlledComponent<ReactProps<PopupProps
overrideProps: this.getContentProps,
})

// Schedules a position update after each render.
// https://popper.js.org/popper-documentation.html#Popper.scheduleUpdate
scheduleUpdate()

return (
<Ref
innerRef={domElement => {
Expand Down