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.

Sign up for GitHub

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

Jump to bottom

PopperContent memory leak #1669

Merged
merged 1 commit into from Oct 18, 2019
Merged

PopperContent memory leak #1669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 2 additions & 16 deletions src/PopperContent.js
View file
Open in desktop
Expand Up @@ -51,7 +51,6 @@ class PopperContent extends React.Component {
constructor(props) {
super(props);

this.handlePlacementChange = this.handlePlacementChange.bind(this);
this.setTargetNode = this.setTargetNode.bind(this);
this.getTargetNode = this.getTargetNode.bind(this);
this.getRef = this.getRef.bind(this);
Expand Down Expand Up @@ -88,13 +87,6 @@ class PopperContent extends React.Component {
this._element = ref;
}

handlePlacementChange(data) {
if (this.state.placement !== data.placement) {
this.setState({ placement: data.placement });
}
return data;
}

onClosed() {
this.props.onClosed();
this.setState({ isOpen: false });
Expand All @@ -120,28 +112,22 @@ class PopperContent extends React.Component {
onClosed,
fade,
transition,
placement,
...attrs
} = this.props;
const arrowClassName = mapToCssModules(classNames(
'arrow',
_arrowClassName
), cssModule);
const placement = this.state.placement || attrs.placement;
const placementFirstPart = placement.split('-')[0];
const popperClassName = mapToCssModules(classNames(
_popperClassName,
placementPrefix ? `${placementPrefix}-${placementFirstPart}` : placementFirstPart
placementPrefix ? `${placementPrefix}-auto` : ''
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows the arrow to inherit its style from the computed popper placement instead of the placement property passed into this component.

See https://github.com/twbs/bootstrap/blob/c1ee395f80c05de8317588b07f34a65c5b95c42c/scss/_popover.scss#L136

), this.props.cssModule);

const extendedModifiers = {
offset: { offset },
flip: { enabled: flip, behavior: fallbackPlacement },
preventOverflow: { boundariesElement },
update: {
enabled: true,
order: 950,
fn: this.handlePlacementChange,
},
...modifiers,
};

Expand Down
60 changes: 9 additions & 51 deletions src/__tests__/PopperContent.spec.js
View file
Open in desktop
Expand Up @@ -103,10 +103,6 @@ describe('PopperContent', () => {
expect(wrapper.find(Popper).props().modifiers).toMatchObject({
// remaining default modifiers
flip: { enabled: true, behavior: 'flip' },
update: {
enabled: true,
order: 950,
},

// additional modifiers
preventOverflow: { boundariesElement: 'viewport' },
Expand All @@ -118,17 +114,19 @@ describe('PopperContent', () => {
wrapper.unmount();
});

it('should have placement class of top by default', () => {
it('should have x-placement of auto by default', () => {
const wrapper = mount(<PopperContent target="target" isOpen container="inline">Yo!</PopperContent>);

expect(wrapper.find('.auto').exists()).toBe(true);
console.log(wrapper.debug());

expect(wrapper.find('div[x-placement="auto"]').exists()).toBe(true);
});

it('should override placement class', () => {
it('should override x-placement', () => {
const wrapper = mount(<PopperContent placement="top" target="target" isOpen container="inline">Yo!</PopperContent>);

expect(wrapper.find('.auto').exists()).toBe(false);
expect(wrapper.find('.top').exists()).toBe(true);
expect(wrapper.find('div[x-placement="auto"]').exists()).toBe(false);
expect(wrapper.find('div[x-placement="top"]').exists()).toBe(true);
});

it('should allow for a placement prefix', () => {
Expand All @@ -140,53 +138,13 @@ describe('PopperContent', () => {
it('should allow for a placement prefix with custom placement', () => {
const wrapper = mount(<PopperContent placementPrefix="dropdown" placement="top" target="target" isOpen container="inline">Yo!</PopperContent>);

expect(wrapper.find('.dropdown-auto').exists()).toBe(false);
expect(wrapper.find('.dropdown-top').exists()).toBe(true);
expect(wrapper.find('.dropdown-auto').exists()).toBe(true);
expect(wrapper.find('div[x-placement="top"]').exists()).toBe(true);
});

it('should render custom tag for the popper', () => {
const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);

expect(wrapper.getDOMNode().tagName.toLowerCase()).toBe('main');
});

it('should handle placement changes from popperjs', () => {
jest.spyOn(PopperContent.prototype, 'setState');
const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);

const instance = wrapper.instance();
const placement = 'top';
expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
instance.handlePlacementChange({ placement });
expect(PopperContent.prototype.setState).toHaveBeenCalled();
expect(wrapper.state('placement')).toBe(placement);

PopperContent.prototype.setState.mockRestore();
});

it('should not update when placement does not change', () => {
jest.spyOn(PopperContent.prototype, 'setState');
const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);

const instance = wrapper.instance();
const placement = 'top';
expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
instance.handlePlacementChange({ placement });
expect(PopperContent.prototype.setState).toHaveBeenCalled();
PopperContent.prototype.setState.mockClear();
instance.handlePlacementChange({ placement });
expect(PopperContent.prototype.setState).not.toHaveBeenCalled();
expect(wrapper.state('placement')).toBe(placement);

PopperContent.prototype.setState.mockRestore();
});

it('should return data from handle placement changes', () => {
const wrapper = mount(<PopperContent tag="main" target="target" isOpen container="inline">Yo!</PopperContent>);

const instance = wrapper.instance();
const data = { placement: 'top' };
const result = instance.handlePlacementChange(data);
expect(result).toEqual(data);
});
});