Skip to content
Merged
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
4 changes: 2 additions & 2 deletions core/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1478,7 +1478,7 @@ export namespace Components {
*/
"columns": GalleryColumns;
/**
* The space between gallery items. Accepts valid CSS [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage) values like `16px`, `1rem`, `20%`, math functions like `calc(10px + 20%)`, or numbers (treated as pixel values). Can also be set as a breakpoint map (e.g. `{ xs: '8px', sm: '1rem', md: '24px' }`). Does not accept space-separated values or CSS keyword values like `inherit`, `auto`, etc.
* The space between gallery items. Accepts valid CSS [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage) values like `16px`, `1rem`, `20%`, math functions like `calc(10px + 20%)`, CSS variables like `var(--app-gallery-gap)`, or numbers (treated as pixel values). Can also be set as a breakpoint map (e.g. `{ xs: '8px', sm: '1rem', md: '24px' }`). Does not accept space-separated values or CSS keyword values like `inherit`, `auto`, etc.
* @default DEFAULT_GAP
*/
"gap": GalleryGap;
Expand Down Expand Up @@ -7526,7 +7526,7 @@ declare namespace LocalJSX {
*/
"columns"?: GalleryColumns;
/**
* The space between gallery items. Accepts valid CSS [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage) values like `16px`, `1rem`, `20%`, math functions like `calc(10px + 20%)`, or numbers (treated as pixel values). Can also be set as a breakpoint map (e.g. `{ xs: '8px', sm: '1rem', md: '24px' }`). Does not accept space-separated values or CSS keyword values like `inherit`, `auto`, etc.
* The space between gallery items. Accepts valid CSS [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage) values like `16px`, `1rem`, `20%`, math functions like `calc(10px + 20%)`, CSS variables like `var(--app-gallery-gap)`, or numbers (treated as pixel values). Can also be set as a breakpoint map (e.g. `{ xs: '8px', sm: '1rem', md: '24px' }`). Does not accept space-separated values or CSS keyword values like `inherit`, `auto`, etc.
* @default DEFAULT_GAP
*/
"gap"?: GalleryGap;
Expand Down
107 changes: 107 additions & 0 deletions core/src/components/gallery/gallery.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,26 @@ describe('gallery', () => {
expect((sharedGallery as any).sanitizeGap('clamp(10px, 20%, 30px)')).toBe('clamp(10px, 20%, 30px)');
});

it('should return undefined for malformed math functions', () => {
const malformedValues = ['calc', 'calc(', 'calc()', 'min(', 'clamp(', 'calc(10px + 20px'];
malformedValues.forEach((value) => {
expect((sharedGallery as any).sanitizeGap(value)).toBeUndefined();
});
});

it('should return the string for CSS variables', () => {
expect((sharedGallery as any).sanitizeGap('var(--app-gap)')).toBe('var(--app-gap)');
expect((sharedGallery as any).sanitizeGap('var(--app-gap, 16px)')).toBe('var(--app-gap, 16px)');
expect((sharedGallery as any).sanitizeGap(' var(--app-gap) ')).toBe('var(--app-gap)');
});

it('should return undefined for malformed CSS variables', () => {
const malformedValues = ['var(--app-gap. 16px)', 'var(--app-gap', 'var()', 'var(16px)'];
malformedValues.forEach((value) => {
expect((sharedGallery as any).sanitizeGap(value)).toBeUndefined();
});
});

it('should return the px value for positive integers', () => {
expect((sharedGallery as any).sanitizeGap(0)).toBe('0px');
expect((sharedGallery as any).sanitizeGap('0')).toBe('0px');
Expand Down Expand Up @@ -613,6 +633,93 @@ describe('gallery', () => {
});
});

it('should resolve to the CSS variable for each breakpoint without warning when gap is a CSS variable', () => {
const breakpoints = DEFAULT_BREAKPOINTS;
const warningSpy = jest.spyOn(logging, 'printIonWarning').mockImplementation(() => {});

sharedGallery.gap = 'var(--app-gap)';

breakpoints.forEach(({ width }) => {
expect((sharedGallery as any).getGapForWidth(width)).toBe('var(--app-gap)');
});

expect(warningSpy).not.toHaveBeenCalled();

warningSpy.mockRestore();
});

it('should resolve to the CSS variable for breakpoints that set one when gap is a breakpoint map', () => {
const breakpoints = [
{ width: 0, expectedGap: DEFAULT_GAP },
{ width: 575, expectedGap: DEFAULT_GAP },
{ width: 576, expectedGap: DEFAULT_GAP },
{ width: 767, expectedGap: DEFAULT_GAP },
{ width: 768, expectedGap: 'var(--app-gap)' },
{ width: 991, expectedGap: 'var(--app-gap)' },
{ width: 992, expectedGap: DEFAULT_GAP },
{ width: 1199, expectedGap: DEFAULT_GAP },
{ width: 1200, expectedGap: DEFAULT_GAP },
{ width: 1399, expectedGap: DEFAULT_GAP },
{ width: 1400, expectedGap: DEFAULT_GAP },
];
const warningSpy = jest.spyOn(logging, 'printIonWarning').mockImplementation(() => {});

sharedGallery.gap = { md: 'var(--app-gap)' };

breakpoints.forEach(({ width, expectedGap }) => {
expect((sharedGallery as any).getGapForWidth(width)).toBe(expectedGap);
});

expect(warningSpy).not.toHaveBeenCalled();

warningSpy.mockRestore();
});

it('should resolve a breakpoint map mixing CSS variables, literals, and unset (default) breakpoints', () => {
const breakpoints = [
{ width: 0, expectedGap: '8px' },
{ width: 575, expectedGap: '8px' },
{ width: 576, expectedGap: 'var(--g-sm)' },
{ width: 767, expectedGap: 'var(--g-sm)' },
{ width: 768, expectedGap: 'var(--g-md)' },
{ width: 991, expectedGap: 'var(--g-md)' },
{ width: 992, expectedGap: DEFAULT_GAP },
{ width: 1199, expectedGap: DEFAULT_GAP },
{ width: 1200, expectedGap: '2rem' },
{ width: 1399, expectedGap: '2rem' },
{ width: 1400, expectedGap: DEFAULT_GAP },
];
const warningSpy = jest.spyOn(logging, 'printIonWarning').mockImplementation(() => {});

sharedGallery.gap = { xs: '8px', sm: 'var(--g-sm)', md: 'var(--g-md)', xl: '2rem' };

breakpoints.forEach(({ width, expectedGap }) => {
expect((sharedGallery as any).getGapForWidth(width)).toBe(expectedGap);
});

expect(warningSpy).not.toHaveBeenCalled();

warningSpy.mockRestore();
});

it('should warn and fallback to the default gap when gap is a malformed CSS variable', () => {
const breakpoints = DEFAULT_BREAKPOINTS;
const warningSpy = jest.spyOn(logging, 'printIonWarning').mockImplementation(() => {});

sharedGallery.gap = 'var(--app-gap. 16px)';

breakpoints.forEach(({ width, expectedGap }) => {
expect((sharedGallery as any).getGapForWidth(width)).toBe(expectedGap);
});

expect(warningSpy).toHaveBeenCalledWith(
expect.stringContaining('[ion-gallery] - Invalid "gap" value ("var(--app-gap. 16px)").'),
el
);

warningSpy.mockRestore();
});

it('should resolve to the proper gap when the gap property is set to an out of order object', () => {
const breakpoints = [
{ width: 0, expectedGap: '8px' },
Expand Down
18 changes: 12 additions & 6 deletions core/src/components/gallery/gallery.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ComponentInterface } from '@stencil/core';
import { Component, Element, Host, Listen, Prop, Watch, h } from '@stencil/core';
import { isValidLengthPercentage } from '@utils/css-value-validation';
import { isCssVariable, isValidLengthPercentage } from '@utils/css-value-validation';
import { raf } from '@utils/helpers';
import { printIonWarning } from '@utils/logging';

Expand Down Expand Up @@ -79,7 +79,8 @@ export class Gallery implements ComponentInterface {
/**
* The space between gallery items. Accepts valid CSS [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage)
* values like `16px`, `1rem`, `20%`, math functions like `calc(10px + 20%)`,
* or numbers (treated as pixel values). Can also be set as a breakpoint map
* CSS variables like `var(--app-gallery-gap)`, or numbers (treated as pixel
* values). Can also be set as a breakpoint map
* (e.g. `{ xs: '8px', sm: '1rem', md: '24px' }`). Does not accept
* space-separated values or CSS keyword values like `inherit`, `auto`, etc.
*/
Expand Down Expand Up @@ -201,9 +202,10 @@ export class Gallery implements ComponentInterface {
}

/**
* Normalize a single gap value (`gap` as a number, string, or one entry from
* a `gap` breakpoint map) to a CSS length string. Returns `undefined` when
* the input cannot be interpreted as a valid CSS length.
* Normalize a single gap value (`gap` as a number, a string such as a CSS
* length-percentage or `var()` reference, or one entry from a `gap`
* breakpoint map) to a CSS length string. Returns `undefined` when the
* input cannot be interpreted as a valid CSS length or `var()` reference.
*/
private sanitizeGap(gap: number | string | undefined): string | undefined {
if (gap === undefined) {
Expand All @@ -224,6 +226,10 @@ export class Gallery implements ComponentInterface {
return undefined;
}

if (isCssVariable(normalizedGap)) {
return normalizedGap;
}

const isValidCssLength = isValidLengthPercentage(normalizedGap);

return isValidCssLength ? normalizedGap : undefined;
Expand Down Expand Up @@ -346,7 +352,7 @@ export class Gallery implements ComponentInterface {
printIonWarning(
`[ion-gallery] - Invalid "gap" value (${JSON.stringify(
gap
)}). Expected a non-negative number, CSS length string, or breakpoint map object (e.g. { xs: 8, md: "1rem" }).`,
)}). Expected a non-negative number, CSS length string, CSS variable (e.g. var(--app-gap)), or breakpoint map object (e.g. { xs: 8, md: "1rem" }).`,
this.el
);
this.hasWarnedInvalidGap = true;
Expand Down
126 changes: 120 additions & 6 deletions core/src/components/gallery/test/basic/gallery.e2e.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A test to verify that the fallback is used when the CSS variable is empty would be beneficial. Not sure if we need to add it to layout/gallery.e2e.ts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import { expect } from '@playwright/test';
import { configs, test } from '@utils/test/playwright';

import { DEFAULT_COLUMNS, DEFAULT_GAP } from '../../gallery-constants';

const DEFAULT_COLUMNS_BREAKPOINTS = [
{ name: 'xs', width: 384, expectedColumns: 2 },
{ name: 'sm', width: 576, expectedColumns: 3 },
{ name: 'md', width: 768, expectedColumns: 4 },
{ name: 'lg', width: 992, expectedColumns: 6 },
{ name: 'xl', width: 1200, expectedColumns: 8 },
{ name: 'xxl', width: 1400, expectedColumns: 10 },
{ name: 'xs', width: 384, expectedColumns: DEFAULT_COLUMNS.xs },
{ name: 'sm', width: 576, expectedColumns: DEFAULT_COLUMNS.sm },
{ name: 'md', width: 768, expectedColumns: DEFAULT_COLUMNS.md },
{ name: 'lg', width: 992, expectedColumns: DEFAULT_COLUMNS.lg },
{ name: 'xl', width: 1200, expectedColumns: DEFAULT_COLUMNS.xl },
{ name: 'xxl', width: 1400, expectedColumns: DEFAULT_COLUMNS.xxl },
];

/**
Expand Down Expand Up @@ -121,6 +123,118 @@ configs({ directions: ['ltr'], modes: ['md'] }).forEach(({ config, screenshot, t
)
.toBe(`${breakpoint.expectedColumns}`);
});

test(`should resolve the default gap value on ${breakpoint.name} screens`, async ({ page }) => {
await page.setViewportSize({ width: breakpoint.width, height: 900 });

await page.setContent(
`
<ion-gallery>
<img src="/src/components/gallery/test/assets/01.png" alt="One" />
<img src="/src/components/gallery/test/assets/02.png" alt="Two" />
<img src="/src/components/gallery/test/assets/03.png" alt="Three" />
<img src="/src/components/gallery/test/assets/04.png" alt="Four" />
</ion-gallery>
`,
config
);

const gallery = page.locator('ion-gallery');

await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).rowGap)).toBe(DEFAULT_GAP);
await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).columnGap)).toBe(DEFAULT_GAP);
});

test(`should resolve the gap CSS variable on ${breakpoint.name} screens`, async ({ page }) => {
await page.setViewportSize({ width: breakpoint.width, height: 900 });

await page.setContent(
`
<ion-gallery style="--app-gap: 24px" gap="var(--app-gap)">
<img src="/src/components/gallery/test/assets/01.png" alt="One" />
<img src="/src/components/gallery/test/assets/02.png" alt="Two" />
<img src="/src/components/gallery/test/assets/03.png" alt="Three" />
<img src="/src/components/gallery/test/assets/04.png" alt="Four" />
</ion-gallery>
`,
config
);

const gallery = page.locator('ion-gallery');

await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).rowGap)).toBe('24px');
await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).columnGap)).toBe('24px');
});

test(`should resolve a gap breakpoint map of CSS variables on ${breakpoint.name} screens`, async ({ page }) => {
await page.setViewportSize({ width: breakpoint.width, height: 900 });

await page.setContent(
`
<ion-gallery style="--g-xs: 2px; --g-sm: 4px; --g-md: 8px; --g-lg: 16px; --g-xl: 24px; --g-xxl: 32px">
<img src="/src/components/gallery/test/assets/01.png" alt="One" />
<img src="/src/components/gallery/test/assets/02.png" alt="Two" />
<img src="/src/components/gallery/test/assets/03.png" alt="Three" />
<img src="/src/components/gallery/test/assets/04.png" alt="Four" />
</ion-gallery>
`,
config
);

const gallery = page.locator('ion-gallery');

// Breakpoint maps are objects, so they are set as a property rather
// than an attribute.
await gallery.evaluate((el) => {
(el as HTMLIonGalleryElement).gap = {
xs: 'var(--g-xs)',
sm: 'var(--g-sm)',
md: 'var(--g-md)',
lg: 'var(--g-lg)',
xl: 'var(--g-xl)',
xxl: 'var(--g-xxl)',
};
});

// The resolved gap for each breakpoint, matching the variables set in
// the style above.
const expectedGap: Record<string, string> = {
xs: '2px',
sm: '4px',
md: '8px',
lg: '16px',
xl: '24px',
xxl: '32px',
};

// Each breakpoint resolves its own gap variable.
await expect
.poll(() => gallery.evaluate((el) => getComputedStyle(el).rowGap))
.toBe(expectedGap[breakpoint.name]);
});
});

test('should resolve the gap CSS variable fallback when the variable is not defined', async ({ page }) => {
await page.setViewportSize({ width: 768, height: 900 });

// The CSS variable `--app-gap` is never declared, so the browser
// resolves the var() fallback (8px).
await page.setContent(
`
<ion-gallery gap="var(--app-gap, 8px)">
<img src="/src/components/gallery/test/assets/01.png" alt="One" />
<img src="/src/components/gallery/test/assets/02.png" alt="Two" />
<img src="/src/components/gallery/test/assets/03.png" alt="Three" />
<img src="/src/components/gallery/test/assets/04.png" alt="Four" />
</ion-gallery>
`,
config
);

const gallery = page.locator('ion-gallery');

await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).rowGap)).toBe('8px');
await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).columnGap)).toBe('8px');
});
});
});
35 changes: 35 additions & 0 deletions core/src/components/gallery/test/layout/gallery.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,4 +366,39 @@ configs({ directions: ['ltr'], modes: ['md'] }).forEach(({ config, screenshot, t
});
});
});

test.describe(title('gallery: masonry gap'), () => {
test('should resolve the gap CSS variable in the masonry layout', async ({ page }) => {
await page.setViewportSize({ width: 768, height: 900 });

// Twelve items so the first item is never the last in its column, whose
// bottom margin masonry zeroes out to remove trailing space.
await page.setContent(
`
<ion-gallery layout="masonry" style="--app-gap: 24px" gap="var(--app-gap)">
<div style="height: 40px">One</div>
<div style="height: 80px">Two</div>
<div style="height: 60px">Three</div>
<div style="height: 100px">Four</div>
<div style="height: 50px">Five</div>
<div style="height: 70px">Six</div>
<div style="height: 90px">Seven</div>
<div style="height: 55px">Eight</div>
<div style="height: 75px">Nine</div>
<div style="height: 65px">Ten</div>
<div style="height: 85px">Eleven</div>
<div style="height: 45px">Twelve</div>
</ion-gallery>
`,
config
);

const gallery = page.locator('ion-gallery');

// In the masonry layout the gap variable drives the column gap
// and the spacing below items (margin bottom).
await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el).columnGap)).toBe('24px');
await expect.poll(() => gallery.evaluate((el) => getComputedStyle(el.children[0]).marginBottom)).toBe('24px');
});
});
});
16 changes: 16 additions & 0 deletions core/src/utils/css-value-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ const LENGTH_PERCENTAGE_PATTERN = /^[-+]?(?:\d+\.?\d*|\.\d+)(?:%|[a-z]+)$/i;
// Matches simple `calc` / `min` / `max` / `clamp(...)` functions.
const MATH_FUNCTION_PATTERN = /^(calc|min|max|clamp)\s*\(.+\)$/i;

// Matches a `var(--name)` reference with an optional fallback, e.g.
// `var(--my-gap)` or `var(--my-gap, 16px)`.
const VAR_FUNCTION_PATTERN = /^var\(\s*--[^\s,)]+\s*(?:,[\s\S]*)?\)$/i;

/**
* Returns whether `value` matches the [length-percentage](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/length-percentage)
* syntax. Accepts `<length>` (`<number>` + unit identifier) or `<percentage>` (`<number>%`).
Expand All @@ -24,3 +28,15 @@ export function isValidLengthPercentage(value: string): boolean {

return MATH_FUNCTION_PATTERN.test(v) || LENGTH_PERCENTAGE_PATTERN.test(v);
}

/**
* Returns whether `value` is a single [`var()`](https://developer.mozilla.org/en-US/docs/Web/CSS/var)
* reference, e.g. `var(--my-token)` or `var(--my-token, 16px)`. The referenced
* custom property is resolved by the browser, so the resolved value is not
* validated here.
*
* @param value String value to validate.
*/
export function isCssVariable(value: string): boolean {
return VAR_FUNCTION_PATTERN.test(value.trim());
}
Loading