-
Notifications
You must be signed in to change notification settings - Fork 60
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
BFE-300: Update Pools Banner #322
base: v2
Are you sure you want to change the base?
Conversation
…een width + draw copy on top vs embedding in the banner
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Skipped Deployments
|
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.
src={`${cloudinaryUrl}/station/pools-bg.png`} | ||
alt="Pools Banner" | ||
layout="responsive" | ||
width={1200} | ||
height={600} | ||
className="h-auto w-full object-cover" | ||
className="z-0" |
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.
Please use fill
instead height and width and set the ratio
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.
unfortunately this will throw @bera/hub:dev: ⨯ Internal error: Error: Image with src "https://res.cloudinary.com/duv0g402y/image/upload/f_auto,q_auto/v1/station/pools-bg.png" is missing required "width" property.
OK I've updated this to use the same insets (breakpoints) as exist in the container + updated the mobile design It seems like the design we're using a different aspect ratio for the banner for mobile views, if we really need that we can do it as well (its better to do when we have the final asset really) |
Here are some UI feedback: Screen 1) There is no drop shadow/blur at the top and you can see a border that is not there on figma. Needs to be added to the menu bar. |
thanks! will circle back to this when I have a banner with a taller aspect ratio |
…issue with text colour tag in the wrong level
…tio we can maintain that is responsive.
ok managed to rework this to handle the mobile view per the figma. I think what's left is to set a max width as well / potentially render a the higher res asset edit: OK we're getting a higher res asset ETA soon. |
This:
Things we should consider: