Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zel-kass
left a comment
zel-kass
left a comment
There was a problem hiding this comment.
Last question, I see that react's version is missing its Figma code connect. Is that intentional ?
| iconType='stroked' | ||
| className='absolute top-8 right-8' | ||
| onClick={() => onClose()} | ||
| aria-label={t('components.banner.closeAriaLabel')} |
There was a problem hiding this comment.
[mid]: In the MediaCard pull request, I introduced a streamlined key common.closeAriaLabel. You might want to use it here, and to align with Abla's previous ContentBanner implementation, you could also add 'closeAriaLabel' prop and do something like aria-label={closeAriaLabel || t('common.closeAriaLabel')}
There was a problem hiding this comment.
Fair, but I kinda dislike the closeAriaLabel prop. I think it's too trivial for consumers to wanna want to edit it. Keen on keeping our component props a bit leaner. Regarding the translation key, keeping the "banner subdomain" as it's the standard in other banners, but I like your initiative.
| <div className='flex flex-col gap-4 py-12'>{children}</div> | ||
| </div> | ||
| <div className='relative w-128'> | ||
| <img |
There was a problem hiding this comment.
[mid]: Right now, if the imageUrl is broken or isn't provided, nothing prevents from breaking and showing :
That is something we don't want. you could either handle that with a useState that is controlled by onLoad and onError, adding some style to hide the failing image. Or conditionally render the image with this state and put a useEffect with the imageUrl in dependencies. Finally, I would also probably conditionally render the gradient overlay based on this state. The var would look something like const showImage = imageUrl && !imageLoadError
There was a problem hiding this comment.
This is a nice idea. I'm assuming we keep imageUrl required as this is a ~media~ banner, but we gracefully hide the broken image and show a fallback flat color? I'm also not keen on hiding the gradient as it highlights the close icon regardless. Lastly I have no idea what's wrong with that gradient in the screenshot, must've gotten something the other way around. Will check with Laurine about the fallback.
| * The banner content (MediaBannerTitle, MediaBannerDescription). | ||
| */ | ||
| children: ReactNode; | ||
| } & Omit<ComponentPropsWithRef<'div'>, 'children'>; |
There was a problem hiding this comment.
[low]: Not sure omitting children here is necessary. From what I remember, ComponentPropsWithRef<'div'> already includes children?: ReactNode. If the intent was to make the children mandatory, in this kind of intersection, the result will be required. So no need to Omit, unless I missed something ? 🤔
There was a problem hiding this comment.
The point was to make it mandatory, yeah. And you're right that ComponentPropsWithRef provides optional children, so extending but omitting children allows the required override to stay, no? Doesn't get replaced by the optional default one.
libs/ui-react/src/lib/Components/MediaBanner/MediaBanner.stories.tsx
Outdated
Show resolved
Hide resolved
libs/ui-rnative/src/lib/Components/MediaBanner/MediaBanner.stories.tsx
Outdated
Show resolved
Hide resolved
| }, | ||
| closeButton: { | ||
| position: 'absolute', | ||
| top: 8.5, |
There was a problem hiding this comment.
[low]: You put 8px on react's version, I see on Figma that 8.5 is the reference value. More of a question, do we want to have 1:1 alignment with designs, or do we prefer using our tokens and streamline both implems ?
There was a problem hiding this comment.
Nice spot! TBF that half-pixel is bugging me hard. I'd just do 8px with token for both. Opinion?
| ); | ||
|
|
||
| return ( | ||
| <Pressable lx={lx} style={[styles.container, style]} {...props}> |
There was a problem hiding this comment.
[low]: since no action is attached to the MediaBanner, why did you wrap in a Pressable instead of a Box ?
There was a problem hiding this comment.
You're right to raise this, IMO it felt natural for a banner to provide a press handler. Wouldn't surprise me if this was requested down the line. Other apps like Kraken do it, for instance. Did so as a nice-to-have which should not affect functionality.
2 participants
Closes DLS-595.