Skip to content

Updating gallery layout to use Robs distribution logic from v2#2289

Merged
LFDanLu merged 7 commits into
cardview_followupfrom
gallery_layout_update_min_width
Sep 16, 2021
Merged

Updating gallery layout to use Robs distribution logic from v2#2289
LFDanLu merged 7 commits into
cardview_followupfrom
gallery_layout_update_min_width

Conversation

@LFDanLu
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu commented Sep 4, 2021

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

…the virtualizer is less than the minItem width
@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

need to figure out why the partition doesnt quite work
}

let weightedRatios = ratios.map(ratio => ratio < this.threshold ? ratio + (0.5 * (1 / ratio)) : ratio);
let partition = linearPartition(weightedRatios, rows);
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.

This partitioning doesn't quite respect the minItemSize.height provided, will need to dig into how the linearPartition works. May require a refactor of these calculations but that can be done after this alpha

@LFDanLu LFDanLu changed the title (WIP) Updating gallery layout to use Robs distribution logic from v2 Updating gallery layout to use Robs distribution logic from v2 Sep 7, 2021
@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@snowystinger
Copy link
Copy Markdown
Member

I wonder, should min-width only apply when there is body content (title/actions/description/footer) so that if it's just the image, they look nice and flush? check out google photos gallery view. sometimes the checkbox extends outside the image

@LFDanLu
Copy link
Copy Markdown
Member Author

LFDanLu commented Sep 14, 2021

@snowystinger hm, a bit tricky to do with the currently implementation since the layout doesn't know anything about the card contents but perhaps something to explore with a refactor to the layout as a whole. I also wonder about how it would look if the user mixes cards with content and cards that only have the image

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

// Compute aspect ratios for all of the items, and the total width if all items were on in a single row.
let ratios = [];
let totalWidth = 0;
let minRatio = this.minItemSize.width / this.minItemSize.height;
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.

Note that this still has a bit of an issue where it is possible for the item's height for extremely tall images to be a bit taller than its row peers:
image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

well that doesn't seem good, lets sit down and go through the math together sometime

@LFDanLu LFDanLu merged commit d90bca5 into cardview_followup Sep 16, 2021
@LFDanLu LFDanLu deleted the gallery_layout_update_min_width branch September 16, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants