feat(gallery): add new gallery component#31101
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: core
There was a problem hiding this comment.
I updated this because this rule prevented the images in my assets/ directory from being served on Vercel.
| // -------------------------------------------------- | ||
|
|
||
| :host(.gallery-layout-uniform) { | ||
| gap: var(--ion-gallery-gap, 16px); |
There was a problem hiding this comment.
Gap currently can only be changed by CSS. Should this be a property that can be updated based on breakpoint, like columns?
There was a problem hiding this comment.
Yes, I lean towards it especially seeing that Chakra does that. I do wonder if we should split it into row-gap and column-gap but we can cross that bridge if we get a request for it so gap works for me.
There was a problem hiding this comment.
Added support for gap here: d494fab
I think if we wanted to support column & row we could just make it accept two values rather than doing two separate properties. I don't know, something to consider later.
|
|
||
| await page.setContent( | ||
| ` | ||
| <ion-gallery style="--internal-gallery-columns: 2;"> |
There was a problem hiding this comment.
Overriding the internal CSS variable for columns should have no effect since the columns property takes priority.
There was a problem hiding this comment.
We need to figure out the file structure. We have some files that separate by periods (gallery.spec.ts) and other by hyphens (gallery-interface.ts). Currently, ionic-modular is converting the interface files to periods (gallery-interface.ts -> gallery.interface.ts)
There was a problem hiding this comment.
I think it's fine to separate them with periods. I was just following the interface naming.
| * Warn about an invalid gap value when it is set to a negative number | ||
| * or a breakpoint map object with invalid values. | ||
| */ | ||
| private warnInvalidGap(gap: GalleryGap) { |
There was a problem hiding this comment.
I was able to pass <ion-gallery gap="pink"> and I didn't get a warning.
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! I had some nits/questions, but I don't consider them blocking honestly. Just food for thought, I guess
| * Return all directly slotted HTMLElement children of the gallery. | ||
| */ | ||
| private getItems() { | ||
| return Array.from(this.el.children).filter((child): child is HTMLElement => child instanceof HTMLElement); |
There was a problem hiding this comment.
child instanceof HTMLElement excludes SVGElement, so a top-level <svg> slotted into the gallery never gets gridColumn, gridRowStart, or gridRowEnd. The ::slotted(*) rules still render it, but masonry placement skips it silently. Worth widening to Element (and casting for style access), or is HTML-only an intentional contract?
| ? this.getColumnsFromBreakpointMap(width, columns) | ||
| : this.sanitizeColumns(columns); | ||
|
|
||
| if (hasInvalidBreakpointColumns || (!isBreakpointColumns && sanitizedColumns === undefined)) { |
There was a problem hiding this comment.
columns={} passes isBreakpointMap because it's a non-null, non-array object, and hasInvalidBreakpointMap returns false since the loop never hits a defined value. The warning branch is skipped and DEFAULT_COLUMNS is silently used. Same story for gap={} or an object with only typo'd keys. Should isBreakpointMap && sanitizedColumns === undefined count as invalid here so devs get a heads-up when their config is being ignored?
| // Wait until the next animation frame to warn about unused order | ||
| // to avoid erroneous warnings when the layout and order are updated | ||
| // in the same frame. | ||
| raf(() => { |
There was a problem hiding this comment.
propertiesChanged watches all four props but warnUnusedOrder is only relevant when layout or order change. Scheduling an raf callback for every columns / gap write is wasted work, even though the dedup flag short-circuits the warning itself. Could the raf be gated on the changed prop, or the watcher split so only layout / order trigger it?
3 participants
Issue number: internal
What is the current behavior?
Developers who want to use a masonry layout must rely on third-party solutions or implement their own.
What is the new behavior?
Adds a new
ion-gallerycomponent.Features
layoutproperty:uniform- evenly sized grid rowsmasonry- stacked masonry layoutorderproperty (masonryonly):sequential- preserves DOM orderbest-fit- places items in the shortest columncolumnsproperty:xs,sm,md,lg,xl,xxl)Test Coverage
columnsfallback and normalization caseslayoutwatcher, masonry scheduling, and load-event behaviororderplacement logic (sequentialandbest-fit)Does this introduce a breaking change?
Other information
Basic Preview
Layout Preview
Dev build:
8.8.7-dev.11778680417.1f9234bd