Skip to content

[CL-473] Adjust popup page max width and scroll containers #14853

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+141 −37
Open

[CL-473] Adjust popup page max width and scroll containers #14853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f2d832
[CL-473] Adjust popup page max width and scroll containers
vleague2 May 20, 2025
d9a8013
remove strict mode warning
vleague2 May 20, 2025
a5e9e84
fix strict mode errors
vleague2 May 20, 2025
437e749
Merge branch 'main' into uif/cl-473/popup-page-layout-tweaks
vleague2 May 21, 2025
36a26d5
fix accidentally deleted query selector
vleague2 May 21, 2025
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
20 changes: 20 additions & 0 deletions apps/browser/src/platform/popup/layout/popup-layout.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ page looks nice when the extension is popped out.
`false`.
- `loadingText`
- Custom text to be applied to the loading element for screenreaders only. Defaults to "Loading".
- `disablePadding`
- When `true`, disables the padding of the scrollable region inside of `main`. You will need to
add your own padding to the element you place inside of this area.

Basic usage example:

Expand Down Expand Up @@ -169,6 +172,23 @@ When the browser extension is popped out, the "popout" button should not be pass

<Canvas of={stories.PoppedOut} />

## With Virtual Scroll

If you are using a virtual scrolling container inside of the popup page (aka replacing the default
popup page scrolling container with a virtual scroll container), you'll want to configure the
component in the following ways:

- Use the `disablePadding` input on the `popup-page`
- Add padding and scrollbar to the virtual scrolling element to match the default behavior of the
popup page scroll container, which ensures that the scrollbar is at the far right edge of the
popup
- Add max width to the child of the virtual scroll element, matching the max width breakpoints used
in the `popup-page`

See the code in the example below.

<Canvas of={stories.WithVirtualScrollChild} />

Comment on lines +175 to +191
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, this seems like a non-intentional limitation of the current design of the component. Thoughts?

# Other stories

## Centered Content
Expand Down
103 changes: 95 additions & 8 deletions apps/browser/src/platform/popup/layout/popup-layout.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
Comment on lines -1 to -2
Copy link
Contributor

Choose a reason for hiding this comment

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

✅

import { ScrollingModule } from "@angular/cdk/scrolling";

Check warning on line 1 in apps/browser/src/platform/popup/layout/popup-layout.stories.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/popup/layout/popup-layout.stories.ts#L1 

Added line #L1 was not covered by tests
import { CommonModule } from "@angular/common";
import { Component, importProvidersFrom } from "@angular/core";
import { RouterModule } from "@angular/router";
Expand Down Expand Up @@ -40,6 +39,17 @@
})
class ExtensionContainerComponent {}

@Component({
selector: "extension-popped-container",
template: `
<div class="tw-h-[640px] tw-w-[900px] tw-border tw-border-solid tw-border-secondary-300">
<ng-content></ng-content>
</div>
`,
standalone: true,
})
class ExtensionPoppedContainerComponent {}

Check warning on line 51 in apps/browser/src/platform/popup/layout/popup-layout.stories.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/popup/layout/popup-layout.stories.ts#L51 

Added line #L51 was not covered by tests

@Component({
selector: "vault-placeholder",
template: /*html*/ `
Expand Down Expand Up @@ -318,6 +328,7 @@
CommonModule,
RouterModule,
ExtensionContainerComponent,
ExtensionPoppedContainerComponent,
MockBannerComponent,
MockSearchComponent,
MockVaultSubpageComponent,
Expand All @@ -328,6 +339,11 @@
MockVaultPagePoppedComponent,
NoItemsModule,
VaultComponent,
ScrollingModule,
ItemModule,
SectionComponent,
IconButtonModule,
BadgeModule,
],
providers: [
{
Expand Down Expand Up @@ -511,7 +527,21 @@
const compact = canvasEl.querySelector(
`#${containerId} [data-testid=popup-layout-scroll-region]`,
);

if (!compact) {
// eslint-disable-next-line
console.error(`#${containerId} [data-testid=popup-layout-scroll-region] not found`);
return;

Check warning on line 534 in apps/browser/src/platform/popup/layout/popup-layout.stories.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/popup/layout/popup-layout.stories.ts#L533-L534 

Added lines #L533 - L534 were not covered by tests
}

const label = canvasEl.querySelector(`#${containerId} .example-label`);

if (!label) {
// eslint-disable-next-line
console.error(`#${containerId} .example-label not found`);
return;

Check warning on line 542 in apps/browser/src/platform/popup/layout/popup-layout.stories.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/popup/layout/popup-layout.stories.ts#L541-L542 

Added lines #L541 - L542 were not covered by tests
}

Comment on lines +530 to +544
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the goal of this change?

const percentVisible =
100 -
Math.round((100 * (compact.scrollHeight - compact.clientHeight)) / compact.scrollHeight);
Expand All @@ -526,9 +556,9 @@
render: (args) => ({
props: args,
template: /* HTML */ `
<div class="tw-h-[640px] tw-w-[900px] tw-border tw-border-solid tw-border-secondary-300">
<extension-popped-container>
<mock-vault-page-popped></mock-vault-page-popped>
</div>
</extension-popped-container>
`,
}),
};
Expand Down Expand Up @@ -576,10 +606,9 @@
template: /* HTML */ `
<extension-container>
<popup-page>
<popup-header slot="header" background="alt"
><span class="tw-italic tw-text-main">🤠 Custom Content</span></popup-header
>

<popup-header slot="header" background="alt">
<span class="tw-italic tw-text-main">🤠 Custom Content</span>
</popup-header>
<vault-placeholder></vault-placeholder>
</popup-page>
</extension-container>
Expand Down Expand Up @@ -624,3 +653,61 @@
`,
}),
};

export const WithVirtualScrollChild: Story = {
render: (args) => ({

Check warning on line 658 in apps/browser/src/platform/popup/layout/popup-layout.stories.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/platform/popup/layout/popup-layout.stories.ts#L657-L658 

Added lines #L657 - L658 were not covered by tests
props: { ...args, data: Array.from(Array(20).keys()) },
template: /* HTML */ `
<extension-popped-container>
<popup-page disablePadding>
<popup-header slot="header" pageTitle="Test"> </popup-header>
<mock-search slot="above-scroll-area"></mock-search>
<div
cdkVirtualScrollingElement
class="tw-h-full tw-p-3 bit-compact:tw-p-2 tw-styled-scrollbar"
>
<div class="tw-max-w-screen-sm tw-mx-auto">
<bit-section>
<bit-item-group aria-label="Mock Vault Items">
<cdk-virtual-scroll-viewport itemSize="55">
<bit-item *cdkVirtualFor="let item of data; index as i">
<button type="button" bit-item-content>
<i
slot="start"
class="bwi bwi-globe tw-text-3xl tw-text-muted"
aria-hidden="true"
></i>
{{ i }} of {{ data.length - 1 }}
<span slot="secondary">Bar</span>
</button>

<ng-container slot="end">
<bit-item-action>
<button type="button" bitBadge variant="primary">Fill</button>
</bit-item-action>
<bit-item-action>
<button
type="button"
bitIconButton="bwi-clone"
aria-label="Copy item"
></button>
</bit-item-action>
<bit-item-action>
<button
type="button"
bitIconButton="bwi-ellipsis-v"
aria-label="More options"
></button>
</bit-item-action>
</ng-container>
</bit-item>
</cdk-virtual-scroll-viewport>
</bit-item-group>
</bit-section>
</div>
</div>
</popup-page>
</extension-popped-container>
`,
}),
};
17 changes: 10 additions & 7 deletions apps/browser/src/platform/popup/layout/popup-page.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,28 @@
<main class="tw-flex-1 tw-overflow-hidden tw-flex tw-flex-col tw-relative tw-bg-background-alt">
<ng-content select="[slot=full-width-notice]"></ng-content>
<div
#nonScrollable
class="tw-transition-colors tw-duration-200 tw-border-0 tw-border-b tw-border-solid tw-p-3 bit-compact:tw-p-2"
[ngClass]="{
'tw-invisible !tw-p-0': loading || nonScrollable.childElementCount === 0,
'tw-invisible !tw-p-0 !tw-border-none': loading || nonScrollable.childElementCount === 0,
'tw-border-secondary-300': scrolled(),
'tw-border-transparent': !scrolled(),
}"
>
<ng-content select="[slot=above-scroll-area]"></ng-content>
<div class="tw-max-w-screen-sm tw-mx-auto tw-w-full" #nonScrollable>
<ng-content select="[slot=above-scroll-area]"></ng-content>
</div>
</div>
<div
class="tw-max-w-screen-sm tw-mx-auto tw-overflow-y-auto tw-flex tw-flex-col tw-size-full tw-styled-scrollbar"
class="tw-mx-auto tw-overflow-y-auto tw-flex tw-flex-col tw-size-full tw-styled-scrollbar"
data-testid="popup-layout-scroll-region"
(scroll)="handleScroll($event)"
[ngClass]="{ 'tw-invisible': loading }"
[ngClass]="{
'tw-invisible': loading,
'tw-p-3 bit-compact:tw-p-2': !disablePadding,
}"
>
<div
class="tw-max-w-screen-sm tw-mx-auto tw-flex-1 tw-flex tw-flex-col tw-w-full"
[ngClass]="{ 'tw-p-3 bit-compact:tw-p-2': !disablePadding }"
class="has-[[cdkvirtualscrollingelement]]:tw-max-w-full tw-max-w-screen-sm tw-mx-auto tw-flex-1 tw-flex tw-flex-col tw-w-full"
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This seems fragile. Is Tailwind has just direct descendants?
  2. It seems odd that we are asking consumers to manually configure the component base on its supplied descendants in one place disablePadding, but then automatically checking descendants in another.

>
<ng-content></ng-content>
</div>
Expand Down
8 changes: 0 additions & 8 deletions apps/browser/src/popup/scss/base.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,14 +381,6 @@ app-root {
}
}

// Adds padding on each side of the content if opened in a tab
@media only screen and (min-width: 601px) {
header,
main {
padding: 0 calc((100% - 500px) / 2);
}
}

main:not(popup-page main) {
position: absolute;
top: 44px;
Expand Down
30 changes: 16 additions & 14 deletions apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.html
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does Vault need their own cdkVirtualScrollingElement?

Original file line number Diff line number Diff line change
Expand Up @@ -91,20 +91,22 @@
cdkVirtualScrollingElement
class="tw-h-full tw-p-3 bit-compact:tw-p-2 tw-styled-scrollbar"
>
<app-autofill-vault-list-items></app-autofill-vault-list-items>
<app-vault-list-items-container
[title]="'favorites' | i18n"
[ciphers]="(favoriteCiphers$ | async) || []"
id="favorites"
collapsibleKey="favorites"
></app-vault-list-items-container>
<app-vault-list-items-container
[title]="'allItems' | i18n"
[ciphers]="(remainingCiphers$ | async) || []"
id="allItems"
disableSectionMargin
collapsibleKey="allItems"
></app-vault-list-items-container>
<div class="tw-max-w-screen-sm tw-mx-auto">
<app-autofill-vault-list-items></app-autofill-vault-list-items>
<app-vault-list-items-container
[title]="'favorites' | i18n"
[ciphers]="(favoriteCiphers$ | async) || []"
id="favorites"
collapsibleKey="favorites"
></app-vault-list-items-container>
<app-vault-list-items-container
[title]="'allItems' | i18n"
[ciphers]="(remainingCiphers$ | async) || []"
id="allItems"
disableSectionMargin
collapsibleKey="allItems"
></app-vault-list-items-container>
</div>
</div>
</ng-container>
</popup-page>
Loading