-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
[FGND] Add gallery image count to Card
#104
Conversation
@ab-gnm has published a preview version of this PR with release workflow run #60, based on commit ea875b5: 1.0.24-PREVIEW.abfgndadd-gallery-image-count-to-card.2024-09-12T0755.ea875b5f Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ab/fgnd/add-gallery-image-count-to-card branch, or use the GitHub CLI command: gh workflow run release.yml --ref ab/fgnd/add-gallery-image-count-to-card Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
proto/blueprint.proto
Outdated
* Number of images in a gallery card. This is used to display gallery | ||
* metadata on card when the media type is MEDIA_TYPE_IMAGE. | ||
*/ | ||
optional int32 gallery_image_count = 46; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think if it would be good to have this field in the Article
message rather than Card
? It feels like something more about the content of the gallery article. In terms of code structure, we can extract this piece of data from the item content which does not depend on the layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've moved it to Article.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually increment the field number by one so I am wondering if we have any reasons to assign 46
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waisingyiu fixed. It was me being lazy - I just copied the declaration from Card
to Article
without changing the field number 🤦🏽♂️
@ab-gnm has published a preview version of this PR with release workflow run #61, based on commit 8731540: 1.0.24-PREVIEW.abfgndadd-gallery-image-count-to-card.2024-09-12T0853.8731540e Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ab/fgnd/add-gallery-image-count-to-card branch, or use the GitHub CLI command: gh workflow run release.yml --ref ab/fgnd/add-gallery-image-count-to-card Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
@ab-gnm has published a preview version of this PR with release workflow run #65, based on commit d66f640: 1.0.24-PREVIEW.abfgndadd-gallery-image-count-to-card.2024-09-12T1729.d66f6401 Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the ab/fgnd/add-gallery-image-count-to-card branch, or use the GitHub CLI command: gh workflow run release.yml --ref ab/fgnd/add-gallery-image-count-to-card Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great, thank you Adi.
What does this change?
Adds a new property to
Article
model for the number of images in a gallery. This allows displaying this count in the gallery pill on cards.Figma: https://www.figma.com/design/kSmjgtoTWiG8N7HXxFoGEE/%E2%97%90-Apps-library?node-id=5245-764&node-type=frame&t=GOWCl1sJcbiISp1k-11