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.

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

[JR] Bugfix/restore original container style on destroy #104

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "adjs",
"version": "2.0.0-beta.24",
"version": "2.0.0-beta.25",
"description": "Ad Library to simplify and optimize integration with ad networks such as DFP",
"main": "./core.js",
"types": "./types.d.ts",
Expand Down
16 changes: 12 additions & 4 deletions src/plugins/Sticky.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class Sticky extends GenericPlugin {
public listener?: any;
public boundary?: IBoundary;
public originalStyle?: any;
public originalStyleCssText?: any;

public onCreate() {
if (!this.isEnabled('sticky')) {
Expand All @@ -31,7 +32,16 @@ class Sticky extends GenericPlugin {

const { container, configuration } = this.ad;
const { stickyOffset = 0 } = configuration;
this.originalStyle = container.style;

this.originalStyle = {};

// Clone to avoid container.style value assigment overriding original style values
Object.entries(container.style).forEach(([_, cssProperty]) => {
this.originalStyle[cssProperty] = container.style.getPropertyValue(cssProperty);
});

// Simplify style restoration on destroy
this.originalStyleCssText = container.style.cssText;

container.style.position = 'sticky';
container.style.top = `${stickyOffset}px`;
Expand All @@ -58,9 +68,7 @@ class Sticky extends GenericPlugin {
public cleanup() {
const { container } = this.ad;

container.style.position = this.originalStyle.position;
container.style.top = this.originalStyle.top;
container.style.transform = this.originalStyle.transform;
container.style.cssText = this.originalStyleCssText;

if (this.listener) {
window.removeEventListener('scroll', this.listener);
Expand Down
42 changes: 41 additions & 1 deletion tests/plugins/Sticky.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,45 @@ describe('Sticky', () => {
const sticky: any = new Sticky(ad);
sticky.onCreate();
expect(ad.container.style.top).toBe('3px');
})
});

it('reverts sticky container to original styles when destroyed', () => {
const dummyParent: any = document.createElement('div');
const dummyAdContainer: any = document.createElement('div');
const originalDummyAdContainerPosition = 'relative'
dummyAdContainer.style.position = originalDummyAdContainerPosition;

dummyParent.appendChild(dummyAdContainer);

const ad = {
configuration: {
breakpoints: {},
stickyOffset: 3,
sticky: true
},
container: dummyAdContainer,
el: {},
network: 'DFP',
render: () => {},
refresh: jest.fn(() => {}),
clear: () => {},
destroy: () => {},
freeze: () => {},
unfreeze: () => {}
}

// @ts-ignore
const sticky: any = new Sticky(ad);

// Container set to sticky
sticky.onCreate();

// Ad container styles like position should be overridden
expect(ad.container.style.position).not.toEqual(originalDummyAdContainerPosition);

// Clean up should revert styles to original
sticky.onDestroy();

expect(ad.container.style.position).toEqual(originalDummyAdContainerPosition);
});
});