Skip to content

feat: add simpler API for controlling overlays #422

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

Closed
wants to merge 2 commits into from
Closed

feat: add simpler API for controlling overlays #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3b1ff2
feat: add simpler API for controlling overlays
cuberoot Jan 9, 2020
a7ed501
chore: put back caching of overlays in array
cuberoot Jan 9, 2020
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
38 changes: 17 additions & 21 deletions packages/overlay/src/active-overlay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
Placement,
OverlayOpenDetail,
TriggerInteractions,
} from './overlay.js';
} from './overlay-types.js';
import calculatePosition, { PositionResult } from './calculate-position.js';
import { Size, Color } from '@spectrum-web-components/theme';
import {
Expand Down Expand Up @@ -114,7 +114,7 @@ export class ActiveOverlay extends LitElement {
public trigger?: HTMLElement;

private placeholder?: Comment;
private root?: HTMLElement;
private root?: HTMLElement = document.body;
Copy link
Contributor

@Westbrook Westbrook Jan 9, 2020

Choose a reason for hiding this comment

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

If we can have this set on the class, can we remove the ?? It would also allow us to remove

spectrum-web-components/packages/overlay/src/active-overlay.ts

Line 249 in e3b1ff2

!this.root ||


@property()
public _state = stateTransition();
Expand Down Expand Up @@ -157,9 +157,9 @@ export class ActiveOverlay extends LitElement {
return [styles];
}

private open(openEvent: CustomEvent<OverlayOpenDetail>): void {
this.extractEventDetail(openEvent);
this.stealOverlayContent(openEvent.detail.content);
private open(openDetail: OverlayOpenDetail): void {
this.extractDetail(openDetail);
this.stealOverlayContent(openDetail.content);

/* istanbul ignore if */
if (!this.overlayContent) return;
Expand All @@ -169,7 +169,7 @@ export class ActiveOverlay extends LitElement {
this.timeout = window.setTimeout(() => {
this.state = 'visible';
delete this.timeout;
}, openEvent.detail.delay);
}, openDetail.delay);

this.hiddenDeferred = new Deferred<void>();
this.addEventListener('animationend', this.onAnimationEnd);
Expand All @@ -178,14 +178,14 @@ export class ActiveOverlay extends LitElement {
});
}

private extractEventDetail(event: CustomEvent<OverlayOpenDetail>): void {
this.overlayContent = event.detail.content;
this.trigger = event.detail.trigger;
this.placement = event.detail.placement;
this.offset = event.detail.offset;
this.interaction = event.detail.interaction;
this.color = event.detail.theme.color;
this.size = event.detail.theme.size;
private extractDetail(detail: OverlayOpenDetail): void {
this.overlayContent = detail.content;
this.trigger = detail.trigger;
this.placement = detail.placement;
this.offset = detail.offset;
this.interaction = detail.interaction;
this.color = detail.theme.color;
this.size = detail.theme.size;
}

public dispose(): void {
Expand Down Expand Up @@ -327,16 +327,12 @@ export class ActiveOverlay extends LitElement {
return this.hasTheme ? this.renderTheme(content) : content;
}

public static create(
openEvent: CustomEvent<OverlayOpenDetail>,
root: HTMLElement
): ActiveOverlay {
public static create(details: OverlayOpenDetail): ActiveOverlay {
const overlay = new ActiveOverlay();

/* istanbul ignore else */
if (openEvent.detail.content) {
overlay.root = root;
overlay.open(openEvent);
if (details.content) {
overlay.open(details);
}

return overlay;
Expand Down
2 changes: 1 addition & 1 deletion packages/overlay/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import { OverlayTrigger } from './overlay-trigger.js';
import { ActiveOverlay } from './active-overlay.js';

export * from './overlay.js';
export * from './overlay-root.js';
export * from './overlay-trigger.js';
export * from './overlay-types';

/* istanbul ignore else */
if (!customElements.get('overlay-trigger')) {
Expand Down
69 changes: 0 additions & 69 deletions packages/overlay/src/overlay-root.ts
View file
Open in desktop

This file was deleted.

53 changes: 27 additions & 26 deletions packages/overlay/src/overlay-stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ governing permissions and limitations under the License.
*/

import { ActiveOverlay } from './active-overlay.js';
import { OverlayOpenDetail, OverlayCloseDetail } from './overlay.js';
import { OverlayOpenDetail } from './overlay-types';

function isLeftClick(event: MouseEvent): boolean {
return event.button === 0;
Expand All @@ -26,15 +26,9 @@ export class OverlayStack {

private preventMouseRootClose = false;
private root: HTMLElement = document.body;
private onChange: (overlays: ActiveOverlay[]) => void;
private handlingResize = false;

public constructor(
root: HTMLElement,
onChange: (overlays: ActiveOverlay[]) => void
) {
this.root = root;
this.onChange = onChange;
public constructor() {
this.addEventListeners();
}

Expand All @@ -46,6 +40,16 @@ export class OverlayStack {
return this.overlays.slice(-1)[0];
}

private findOverlayForContent(
overlayContent: HTMLElement
): ActiveOverlay | undefined {
for (const item of this.overlays) {
if (overlayContent.isSameNode(item.overlayContent as HTMLElement)) {
return item;
}
}
}

private addEventListeners(): void {
this.document.addEventListener('click', this.handleMouseCapture, true);
this.document.addEventListener('click', this.handleMouse);
Expand All @@ -67,34 +71,30 @@ export class OverlayStack {
);
}

public openOverlay(event: CustomEvent<OverlayOpenDetail>): void {
if (this.isOverlayActive(event.detail.content)) return;
public openOverlay(details: OverlayOpenDetail): void {
/* istanbul ignore if */
if (this.isOverlayActive(details.content)) return;

requestAnimationFrame(() => {
const interaction = event.detail.interaction;
if (interaction === 'click') {
if (details.interaction === 'click') {
this.closeAllHoverOverlays();
} else if (
interaction === 'hover' &&
this.isClickOverlayActiveForTrigger(event.detail.trigger)
details.interaction === 'hover' &&
this.isClickOverlayActiveForTrigger(details.trigger)
) {
// Don't show a hover popover if the click popover is already active
return;
}

const activeOverlay = ActiveOverlay.create(event, this.root);
const activeOverlay = ActiveOverlay.create(details);
this.overlays.push(activeOverlay);

this.onChange(this.overlays);
document.body.appendChild(activeOverlay);
Copy link
Contributor

@Westbrook Westbrook Jan 9, 2020

Choose a reason for hiding this comment

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

With the structural changes herein, if I wanted to make the "overlay root" element flexible, where would you see adding that? Having this as a static on Overlay makes me feel like it's a little fixed, but I don't use a lot of statics in my code to be fully aware of the possibilities they afford.

The issue this supports is something that is currently in Crisp where we don't actually use sp-theme to set our styles, as previously there was no way to use it with only some of the styles. The answer may simply be for me to add the large styles to the sp-theme package, but an alternative would be to allow for our crisp-app element to be set for use in place of document.body here by sort sort of configuration.

Copy link
Contributor

@benjamind benjamind Jan 9, 2020

Choose a reason for hiding this comment

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

I too am a little concerned about not being able to control where we reparent overlays to.

The case I'm thinking of is a modal thats contained within an area of an application, in this case I want its z-index to be above the content in the containment area, but below content in other areas outside of the container. This seems natural to me to create an overlay root thats specifically for use by this modal, with the main document.body root being used for tooltips etc.

Is this a case this change supports? I think we may have even lost this capability before this change?

Copy link
Contributor

@Westbrook Westbrook Jan 9, 2020

Choose a reason for hiding this comment

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

This was a feature taken away when removing overlay-root, yes. I don't personally have a preference between needing it now vs the ability to easily refactor this functionality in later, which is why it was omitted previously when this wasn't a static.

@benjamind could you expand on:

The case I'm thinking of is a modal thats contained within an area of an application, in this case I want its z-index to be above the content in the containment area, but below content in other areas outside of the container.

Is this a case of a UI with multiple modals open at the same time, or something else? Would be a good use case to have clearly defined in the docs as a "possible" or "not possible" depending on how this comes together.

Copy link
Contributor

@benjamind benjamind Jan 9, 2020

Choose a reason for hiding this comment

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

The case I was thinking of was something like this:

image

Where we have an area where we want a contained modal, this has a mask area behind the modal and a dialog with tooltips etc inside. This is one stack of overlays.

The application however supports dropdowns and tooltips that can potentially overlap this stack, but do so from outside of the contained area.

I've not delved too deeply into how our overlay stack works at this time, so I'm not sure how we support this today, especially with regards to resize of the masked area etc.

Copy link
Contributor Author

@cuberoot cuberoot Jan 9, 2020

Choose a reason for hiding this comment

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

So, our overlays stack globally in the order in which they are opened. I could see the dialog being an overlay itself. If you opened an overlay in the dialog (tooltip on hover, click popover, etc) it would open at a higher level than the dialog. If you opened a dropdown from the header, while the dialog was open (ignoring that it is supposed to be modal), the dropdown would appear over the dialog as you would expect.

A problem with explicitly restricting the overlay area to a subset of the view (e.g. the mask) is that we may restrict the positioning of something like a dropdown more than we need to. I don't know if that is something that we want?

So, I guess I am saying that I think we would get reasonable results in the scenario you have outlined without multiple levels of overlay root. I am not yet convinced that the added complexity is warranted. But, I could be convinced 😁

Copy link
Contributor

@benjamind benjamind Jan 9, 2020

Choose a reason for hiding this comment

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

Yeah I think I agree that our current code covers the case for the z-stacking issue.

The main concern I have is I think about responding to resize. Since the modal mask and the dialog would be centered within their container, but they are re-parented to document.body (or our overlay root), then I'm not quite sure how they would manage their resizing without having to have very explicit code to manage sizing the modal and dialog against their original container?

});
}

public closeOverlay(event: CustomEvent<OverlayCloseDetail>): void {
public closeOverlay(content: HTMLElement): void {
requestAnimationFrame(() => {
const overlayContent = event.detail.content;
const overlay = this.overlays.find((item) =>
overlayContent.isSameNode(item.overlayContent as HTMLElement)
);
const overlay = this.findOverlayForContent(content);
this.hideAndCloseOverlay(overlay);
});
}
Expand Down Expand Up @@ -133,13 +133,14 @@ export class OverlayStack {
private async hideAndCloseOverlay(overlay?: ActiveOverlay): Promise<void> {
if (overlay) {
await overlay.hide();
overlay.remove();
overlay.dispose();

const index = this.overlays.indexOf(overlay);
/* istanbul ignore else */
if (index >= 0) {
this.overlays[index].dispose();
this.overlays.splice(index, 1);
}
this.onChange(this.overlays);
}
}

Expand All @@ -164,9 +165,9 @@ export class OverlayStack {

this.handlingResize = true;
requestAnimationFrame(() => {
this.overlays.forEach((overlay) => {
for (const overlay of this.overlays) {
overlay.updateOverlayPosition();
});
}
this.handlingResize = false;
});
};
Expand Down
Loading