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: add required vertical space for combo-box overlay #5432

Merged
merged 9 commits into from
Feb 2, 2023
6 changes: 6 additions & 0 deletions packages/combo-box/src/vaadin-combo-box-overlay.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ export class ComboBoxOverlay extends ComboBoxOverlayMixin(Overlay) {

return memoizedTemplate;
}

constructor() {
super();

this.requiredVerticalSpace = 200;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if 200px is a good default value for combo-box?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a good start. We can consider changing it, or making this configurable if this doesn't fit every use case.

Copy link
Member

Choose a reason for hiding this comment

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

We used to have a hardcoded number in V14 - IIRC, it originates from the 1.x Paper elements based version. It makes no sense already in V10 where we switched to Lumo, but apparently no one noticed it 🙂

Personally, I feel like 200px should be a reasonable default for the minumal overlay height.

}
}

customElements.define(ComboBoxOverlay.is, ComboBoxOverlay);
72 changes: 58 additions & 14 deletions packages/combo-box/test/overlay-position.test.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,9 @@
import { expect } from '@esm-bundle/chai';
import { aTimeout, fixtureSync, isIOS } from '@vaadin/testing-helpers';
import '../src/vaadin-combo-box.js';
import { html, PolymerElement } from '@polymer/polymer/polymer-element.js';
import '../vaadin-combo-box.js';
import './not-animated-styles.js';
import { makeItems, setInputValue } from './helpers.js';

class XFixed extends PolymerElement {
static get template() {
return html`
<div style="position: fixed;">
<slot></slot>
</div>
`;
}
}

customElements.define('x-fixed', XFixed);

describe('overlay position', () => {
let comboBox, overlayPart, inputField;

Expand All @@ -33,6 +21,14 @@ describe('overlay position', () => {
}

beforeEach(async () => {
fixtureSync(`
<style>
vaadin-combo-box-overlay::part(overlay) {
margin: 0 !important;
}
</style>
`);

comboBox = fixtureSync(`<vaadin-combo-box label='comboBox' style='width: 300px;' items='[1]'></vaadin-combo-box>`);
const comboBoxRect = comboBox.getBoundingClientRect();
comboBox.items = makeItems(20);
Expand Down Expand Up @@ -168,6 +164,54 @@ describe('overlay position', () => {
await aTimeout(0);
expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1);
});

it('should be above input when near bottom', async () => {
moveComboBox(xCenter, yBottom - 200, 300);

comboBox.open();
await aTimeout(1);
expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1);
});

it('should be below input when far from bottom', async () => {
moveComboBox(xCenter, yBottom - 220, 300);

comboBox.open();
await aTimeout(1);
expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1);
});

describe('lazy data provider', () => {
beforeEach(() => {
comboBox.items = undefined;

comboBox.dataProvider = (params, callback) => {
const index = params.page * params.pageSize;
const size = 20;
const result = [...Array(size).keys()].map((i) => ({
label: `Item ${index + i}`,
value: `item-${index + i}`,
}));
setTimeout(() => callback(result, size), 100);
};
});

it('should be above input when near bottom', async () => {
moveComboBox(xCenter, yBottom - 200, 300);

comboBox.open();
await aTimeout(1);
expect(overlayPart.getBoundingClientRect().bottom).to.closeTo(inputField.getBoundingClientRect().top, 1);
});

it('should be below input when far from bottom', async () => {
moveComboBox(xCenter, yBottom - 220, 300);

comboBox.open();
await aTimeout(1);
expect(overlayPart.getBoundingClientRect().top).to.closeTo(inputField.getBoundingClientRect().bottom, 1);
});
});
});

describe('overlay resizing', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/overlay/src/vaadin-overlay-position-mixin.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,14 @@ export declare class PositionMixinClass {
* @attr {boolean} no-vertical-overlap
*/
noVerticalOverlap: boolean;

/**
* If the overlay content has no intrinsic height, this property can be used to set
* the minimum vertical space (in pixels) required by the overlay. Setting a value to
* the property effectively disables the content measurement in favor of using this
* fixed value for determining the open direction.
*
* @attr {number} required-vertical-space
*/
requiredVerticalSpace: number;
}
18 changes: 16 additions & 2 deletions packages/overlay/src/vaadin-overlay-position-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,25 @@ export const PositionMixin = (superClass) =>
type: Boolean,
value: false,
},

/**
* If the overlay content has no intrinsic height, this property can be used to set
* the minimum vertical space (in pixels) required by the overlay. Setting a value to
* the property effectively disables the content measurement in favor of using this
* fixed value for determining the open direction.
*
* @attr {number} required-vertical-space
*/
requiredVerticalSpace: {
Copy link
Member Author

Choose a reason for hiding this comment

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

PositionMixin is private so didn't label this as a feat.

Copy link
Member Author

@tomivirkki tomivirkki Feb 1, 2023

Choose a reason for hiding this comment

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

Not 100% sure about the property name. Any other suggestions? It's for internal use only so it doesn't matter all that much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds fine. I would suggest to mention in the JsDoc that this effectively disables the content measurement in favor of using this fixed value for determining the open direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the description

type: Number,
value: 0,
},
};
}

static get observers() {
return [
'__positionSettingsChanged(horizontalAlign, verticalAlign, noHorizontalOverlap, noVerticalOverlap)',
'__positionSettingsChanged(horizontalAlign, verticalAlign, noHorizontalOverlap, noVerticalOverlap, requiredVerticalSpace)',
'__overlayOpenedChanged(opened, positionTarget)',
];
}
Expand Down Expand Up @@ -262,7 +275,8 @@ export const PositionMixin = (superClass) =>
__shouldAlignStartVertically(targetRect) {
// Using previous size to fix a case where window resize may cause the overlay to be squeezed
// smaller than its current space before the fit-calculations.
const contentHeight = Math.max(this.__oldContentHeight || 0, this.$.overlay.offsetHeight);
const contentHeight =
this.requiredVerticalSpace || Math.max(this.__oldContentHeight || 0, this.$.overlay.offsetHeight);
sissbruecker marked this conversation as resolved.
Show resolved Hide resolved
this.__oldContentHeight = this.$.overlay.offsetHeight;

const viewportHeight = Math.min(window.innerHeight, document.documentElement.clientHeight);
Expand Down
32 changes: 32 additions & 0 deletions packages/overlay/test/position-mixin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,22 @@ describe('position mixin', () => {
expectEdgesAligned(TOP, TOP);
});

it('should flip when out of required vertical space', () => {
overlay.requiredVerticalSpace = 200;
target.style.top = `${targetPositionToFlipOverlay - 100}px`;
updatePosition();
expectEdgesAligned(BOTTOM, BOTTOM);
});

it('should flip back when enough required vertical space', () => {
overlay.requiredVerticalSpace = 200;
target.style.top = `${targetPositionToFlipOverlay - 100}px`;
updatePosition();
target.style.top = `${targetPositionToFlipOverlay - 200}px`;
updatePosition();
expectEdgesAligned(TOP, TOP);
});

it('should choose the bigger side when it fits neither', () => {
overlayContent.style.height = `${document.documentElement.clientHeight}px`;

Expand Down Expand Up @@ -262,6 +278,22 @@ describe('position mixin', () => {
expectEdgesAligned(BOTTOM, BOTTOM);
});

it('should flip when out of required vertical space', () => {
overlay.requiredVerticalSpace = 200;
target.style.top = `${targetPositionToFlipOverlay + 100}px`;
updatePosition();
expectEdgesAligned(TOP, TOP);
});

it('should flip back when enough required vertical space', () => {
overlay.requiredVerticalSpace = 200;
target.style.top = `${targetPositionToFlipOverlay + 100}px`;
updatePosition();
target.style.top = `${targetPositionToFlipOverlay + 200}px`;
updatePosition();
expectEdgesAligned(BOTTOM, BOTTOM);
});

it('should choose the bigger side when it fits neither', () => {
overlayContent.style.height = `${document.documentElement.clientHeight}px`;

Expand Down
1 change: 1 addition & 0 deletions packages/overlay/test/typings/overlay.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,4 @@ assertType<boolean>(customOverlay.noVerticalOverlap);
assertType<'end' | 'start'>(customOverlay.horizontalAlign);
assertType<'bottom' | 'top'>(customOverlay.verticalAlign);
assertType<HTMLElement>(customOverlay.positionTarget);
assertType<number>(customOverlay.requiredVerticalSpace);