-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(project): use a native fallback for image service #328
Conversation
Visit the preview URL for this PR (updated for commit 302b23b): https://ottwebapp--pr328-feat-simplify-altern-g1ccvisf.web.app (expires Thu, 17 Aug 2023 15:54:34 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c198f8a3a199ba8747819f7f1e45cf602b777529 |
b4039d9
to
951a950
Compare
951a950
to
0fc9bd6
Compare
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.
Nice cleanup. I've left two small comments/questions
44ebd6e
to
6f993c6
Compare
src/components/CardGrid/CardGrid.tsx
Outdated
@@ -55,7 +55,7 @@ function CardGrid({ | |||
onCardHover, | |||
}: CardGridProps) { | |||
const breakpoint: Breakpoint = useBreakpoint(); | |||
const posterAspect = parseAspectRatio(playlist.shelfImageAspectRatio); | |||
const posterAspect = parseAspectRatio(playlist.aspectRatio || playlist.shelfImageAspectRatio); |
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.
@ChristiaanScheermeijer Do you know any cases where shelfImageAspectRatio
is used by customers? I would just leave aspectRatio
.
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.
I don't mind keeping the more verbose param. What was the reason for the name change in the first place? Just aspectRatio
might imply the sizing of the shelf itself.
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 use now cardImage
instead of shelfImage
as we changed the label to card
by default (AC). Previously shelfImage
custom param was used to define the label. I can also use cardImageAspectRatio
.
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.
I don't think so. We're using this in the demo app configs, though. But if you are going to change it and remove the fallback, it would be good to mention it in the changelogs.
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.
Left the fallback withshelfImageAspectRatio
(deprecated), added cardImageAspectRatio
- deprecate `shelfImageAspectRatio` custom param - add `cardImageAspectRatio` custom param
Description
poster_fallback=1
query)This PR solves # .
Steps completed:
According to our definition of done, I have completed the following steps: