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

fix(popup): now watching anchor size for position correctness #1236

Merged
merged 9 commits into from
Feb 15, 2022
16 changes: 6 additions & 10 deletions components/popup/src/vwc-popup-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,8 @@ export class VWCPopupBase extends LitElement {
this.open = false;
}

// TODO: Make the observer work on positioning for the first time
// new `IntersectionObserver` constructor
private observer = new IntersectionObserver((entries) => {
for (const entry of entries) {
const bounds = entry.boundingClientRect;
console.log('anchor element bounds ', bounds);
requestAnimationFrame(() => this.updatePosition());
}
private sizeObserver = new ResizeObserver(() => {
return this.updatePosition();
});

override connectedCallback(): void {
Expand All @@ -143,19 +137,21 @@ export class VWCPopupBase extends LitElement {
window.removeEventListener('scroll', this.updatePosition);
window.removeEventListener('resize', this.onResizeWindow);
// Disconnect the observer to stop from running in the background
this.observer.disconnect();
this.sizeObserver.disconnect();
}

protected override firstUpdated(_changedProperties: PropertyValues): void {
super.firstUpdated(_changedProperties);
this.anchorEl = this.getAnchorById();
if(this.anchorEl) this.observer.observe(this.anchorEl);
if(this.anchorEl) this.sizeObserver.observe(this.anchorEl);
}

protected override updated(changes: Map<string, boolean>): void {
super.updated(changes);
if (changes.has('anchor')) {
this.sizeObserver.disconnect();
this.anchorEl = this.getAnchorById();
if(this.anchorEl) this.sizeObserver.observe(this.anchorEl);
}
this.updatePosition();
}
Expand Down
9 changes: 5 additions & 4 deletions components/popup/stories/popup.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const Template = args => html`
}
</style>
<div class="popup-wrapper">
<vwc-button id="buttonAnchor" layout="outlined" outlined aria-haspopup="true" aria-describedby="popup" @click=${onClick}>Click to open popup</vwc-button>
<vwc-button id="buttonAnchor" layout="outlined" outlined aria-haspopup="true" aria-describedby="popup" @click=${onClick}>Click on me</vwc-button>
<vwc-popup id="popup" anchor="buttonAnchor" ...=${spread(args)}>
<div class="content">
<vwc-text font-face="body-1-bold" tight><p class="line">Popup title</p></vwc-text>
Expand All @@ -45,15 +45,16 @@ const Template = args => html`
`;

export const Basic = Template.bind({});
Basic.args = { open: true };

export const WithArrow = Template.bind({});
WithArrow.args = { arrow: true, corner: "right" };
WithArrow.args = { open: true, arrow: true, corner: "right" };

export const Alternate = Template.bind({});
Alternate.args = { arrow: true, corner: "bottom", alternate: true };
Alternate.args = { open: true, arrow: true, corner: "bottom", alternate: true };

export const Dismissible = Template.bind({});
Dismissible.args = { dismissible: true, corner: "top" };
Dismissible.args = { open: true, dismissible: true, corner: "top" };

function onClick() {
popup.open = !popup.open;
Expand Down
81 changes: 73 additions & 8 deletions components/popup/test/popup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import 'chai-dom';
import {
waitNextTask,
textToDomToParent,
isolatedElementsCreation,
isolatedElementsCreation, waitInterval, noop,
Copy link
Contributor

Choose a reason for hiding this comment

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

is waitInterval needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not. Don't we have a linting rule that should fail if unused variable?
Anyway - if it doesn't appear in the code, it can be removed.

} from '../../../test/test-helpers.js';
import { chaiDomDiff } from '@open-wc/semantic-dom-diff';
import {chaiDomDiff} from '@open-wc/semantic-dom-diff';

chai.use(chaiDomDiff);

Expand Down Expand Up @@ -39,7 +39,7 @@ describe('popup', () => {
await actualElement.updateComplete;
expect(actualElement.anchor, 'anchor should be ""')
.to
.equal("");
.equal('');
expect(actualElement.open, 'open should be false')
.to
.equal(false);
Expand All @@ -66,7 +66,7 @@ describe('popup', () => {
const [actualElement] = addElement(
textToDomToParent(`<${COMPONENT_NAME}></${COMPONENT_NAME}>`)
);
actualElement.anchor = "anchor";
actualElement.anchor = 'anchor';
await actualElement.updateComplete;

actualElement.show();
Expand Down Expand Up @@ -94,11 +94,40 @@ describe('popup', () => {
});

describe(`anchor`, () => {
it(`should not open the popup if anchor does not exist`, async () => {
async function flushResizeEvents() {
await waitNextTask();
}

function triggerResize(element) {
element.style.height = '20px';
}

function expectASingleUpdatePositionCall(updatePositionCallCount, anchorElement) {
const observer = new ResizeObserver(async function () {
await waitNextTask();
expect(updatePositionCallCount).to.equal(1);
});

observer.observe(anchorElement);
}

async function setPopupAndAnchor() {
const [anchorElement] = addElement(
textToDomToParent(`<vwc-button style="position: absolute; left: 100px;" layout="outlined" id="anchor">Button</vwc-button>`)
);
const [actualElement] = addElement(
textToDomToParent(`<${COMPONENT_NAME} arrow anchor="anchor" open><div>This is my popup</div></${COMPONENT_NAME}>`)
);
actualElement.updatePosition = noop;
await actualElement.updateComplete;
return {anchorElement, actualElement};
}

it(`should not set popup open if anchor element does not exist`, async () => {
const [actualElement] = addElement(
textToDomToParent(`<${COMPONENT_NAME}></${COMPONENT_NAME}>`)
);
actualElement.anchor = "anchor";
actualElement.anchor = 'anchor';
await actualElement.updateComplete;

actualElement.show();
Expand All @@ -109,16 +138,52 @@ describe('popup', () => {
.equal(false);
});

it(`should not open the popup if anchor does not exist`, async () => {
it(`should init the popup as open if anchor element does not exist`, async () => {
const [actualElement] = addElement(
textToDomToParent(`<${COMPONENT_NAME} open></${COMPONENT_NAME}>`)
);
actualElement.anchor = "anchor";
actualElement.anchor = 'anchor';
await actualElement.updateComplete;

expect(actualElement.open)
.to
.equal(false);
});

it(`should reposition when the anchor changes its size`, async function () {

const {anchorElement, actualElement} = await setPopupAndAnchor();

await flushResizeEvents();

let updatePositionCallCount = 0;
actualElement.updatePosition = function () {
updatePositionCallCount++;
};

triggerResize(anchorElement);

expectASingleUpdatePositionCall(updatePositionCallCount, anchorElement);

});

it(`should stop observing the anchor when changing an anchor`, async function () {
const {anchorElement, actualElement} = await setPopupAndAnchor();
await flushResizeEvents();

actualElement.anchor = '';
await waitNextTask();

let updatePositionCallCount = 0;
actualElement.updatePosition = function () {
updatePositionCallCount++;
};

anchorElement.style.height = '20px';

await waitNextTask();

expect(updatePositionCallCount).to.equal(0);
});
});
});
9 changes: 7 additions & 2 deletions test/test-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@ export {
isFirefox,
getFrameLoadedInjected,
cleanFrame,
getRandom
getRandom,
noop
}

const noop = () => {
const x = [];
x.push(5);
};
const tmpTemple = document.createElement('template');

function listenToSubmission(formElement) {
Expand Down Expand Up @@ -289,4 +294,4 @@ class TestComponent extends HTMLElement {
}
}

window.customElements.define('vivid-tests-component', TestComponent);
window.customElements.define('vivid-tests-component', TestComponent);
1 change: 1 addition & 0 deletions ui-tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ async function setDevServer() {
open: true
};

//@ts-ignore
const devServer = new WebpackDevServer(devServerOptions, compiler);

devServer.listen(webpackConfig.devServer.port, '127.0.0.1', () => {
Expand Down
Binary file modified ui-tests/snapshots/vwc-popup.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
45 changes: 45 additions & 0 deletions ui-tests/tests/vwc-popup/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import '@vonage/vwc-popup';
import '@vonage/vwc-button';
import '@vonage/vwc-text';

export async function createElementVariations(wrapper) {
const textElementWrapper = document.createElement('div');
textElementWrapper.innerHTML = `
<style>
.popup-wrapper {
width: 100%;
height: 400px;
display: flex;
align-items: center;
justify-content: center;
background-color: var(--vvd-color-neutral-10);
}
.content {
width: 200px;
text-align: left;
padding: 1rem;
}
.line {
border-bottom: 1px solid var(--vvd-color-neutral-40);
padding-bottom: 0.5rem;
margin-bottom: 0.5rem;
}
</style>
<div class="popup-wrapper">
<vwc-button id="buttonAnchor" layout="outlined" outlined aria-haspopup="true" aria-describedby="popup">Click to open popup</vwc-button>
<vwc-popup id="popup" open corner="left" dismissible anchor="buttonAnchor">
<div class="content">
<vwc-text font-face="body-1-bold" tight><p class="line">Popup title</p></vwc-text>
<vwc-text font-face="body-2" tight>Lorem ipsum dolor sit amet, consectetur adipiscing elit.</vwc-text>
</div>
</vwc-popup>
<vwc-popup arrow alternate corner="right" id="popup" open anchor="buttonAnchor">
<div class="content">
<vwc-text font-face="body-1-bold" tight><p class="line">Popup title</p></vwc-text>
<vwc-text font-face="body-2" tight>Lorem ipsum dolor sit amet, consectetur adipiscing elit.</vwc-text>
</div>
</vwc-popup>
</div>`;
wrapper.appendChild(textElementWrapper);

}