Conversation
|
Size stats
|
|
Accessibility report ℹ️ You can run this locally by executing |
|
Deploy preview for mistica-web ready! ✅ Preview Built with commit d465585. |
| borderRadius: skinVars.borderRadii.container, | ||
| }), | ||
| { | ||
| zIndex: 1, |
There was a problem hiding this comment.
I don't know why we had this, but removing it didn't break any screenshot test (also tested in webapp)
| @@ -0,0 +1,35 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
extracted this part to a different file because it's te interactive part of the component, hence we need "use client". This way the rest of the component is a RSC and will not contribute to bundle size in the browser.
| left, | ||
| right, | ||
| verticalSpace, | ||
| collapseBreakpoint = 'tablet', |
There was a problem hiding this comment.
this prop is new, this will allow configuring GridLayout to collapse in mobile breackpoint instead of tablet (default behaviour). This mode is required in CoverHero
| default: vars.colors.background, | ||
| inverse: vars.colors.backgroundBrand, | ||
| alternative: vars.colors.backgroundAlternative, | ||
| }[variant ?? 'default']; |
There was a problem hiding this comment.
This is not how we handle variants in Mistica. For example, this playroom looks weird to me. All the other components read the variant from the context
There was a problem hiding this comment.
no, you have to distinguish between components that a adapt to external variant and components that sets the variant. For example, a button or a text component adapt the colors depending on the external variant context, but Hero, Header or some Cards can set their own variant
src/cover-hero.tsx
Outdated
| minHeight: aspectRatio && aspectRatio !== 'auto' ? 'auto' : minHeight, | ||
| boxSizing: 'border-box', | ||
| background, | ||
| aspectRatio: aspectRatioToCssString(aspectRatio), |
There was a problem hiding this comment.
aspectRatio is not supported by all the Chrome versions we need: https://caniuse.com/?search=aspect-ratio
Why not using AspectRatioContainer (doing something similar to what we do with cards)
There was a problem hiding this comment.
I wanted to avoid the paddingTop hack because this is a rare case in this component and the graceful degradation for old browsers is quite good here.
Anyway, I finally included the hack, although I needed an extra div :(
| const CardActionPlayIcon = ({color}: IconProps) => <IconPlayFilled color={color} size={12} />; | ||
|
|
||
| const useVideoWithControls = ( | ||
| export const useVideoWithControls = ( |
There was a problem hiding this comment.
should we abstract this in a separate file instead? It's not something specific to cards anymore
There was a problem hiding this comment.
Yes I thought about it, I can extract it if you prefer
There was a problem hiding this comment.
Yes, I think we should do it in order to keep the code well organized
There was a problem hiding this comment.
I think we could create a top-actions.tsx file with all the top actions components/hooks including this one, what do you think?
There was a problem hiding this comment.
talked offline, we'll move this refactor to the future (when we have over media variant)
src/cover-hero.tsx
Outdated
| <Box paddingY={noPaddingY ? 0 : {desktop: 56, tablet: 56, mobile: 24}}> | ||
| <Stack space={24}> | ||
| {centered && !sideExtra ? ( | ||
| mainContent |
src/__stories__/cover-hero-story.tsx
Outdated
| options: ['default', 'inverse', 'alternative'], | ||
| control: {type: 'select'}, | ||
| // this control should only be visible when background is set to 'color from skin' or 'custom color' | ||
| // if: {arg: 'background', eq: 'color'}, |
There was a problem hiding this comment.
This is a missing storybook feature. I can configure the variant control to appear when background is 'color from skin': if: {arg: 'background', eq: 'color from skin'} but I can't configure it to appear when background is 'color from skin' or 'custom color'
There was a problem hiding this comment.
Yes, it's a very annoying thing from Storybook, but why leaving this commented line? I'd remove it
There was a problem hiding this comment.
just to document the intention, but I can remove it
There was a problem hiding this comment.
I've rewritten the comment with a better explanation and added a reference to this PR: ComponentDriven/csf#76
# [15.11.0](v15.10.0...v15.11.0) (2024-06-19) ### Bug Fixes * **ButtonLayout:** add bleed when using only link ([#1150](#1150)) ([554f98a](554f98a)) * **Counter:** add aria-live to value ([#1146](#1146)) ([3e2e09b](3e2e09b)) * **Header:** bleed not working in o2-new skin ([#1137](#1137)) ([00fb632](00fb632)) * **MainNavigationBar:** remove logo space in mobile when no sections are given ([#1149](#1149)) ([e4c03a0](e4c03a0)) * **Select:** set text color in native version ([#1141](#1141)) ([eedf265](eedf265)) * **Spinner:** use controlActivatedInverse token as default when used inside inverse variant ([#1133](#1133)) ([38a192d](38a192d)) ### Features * **Cards:** improve accessibility ([#1139](#1139)) ([dde9cc5](dde9cc5)) * **Chip:** allow using badge in selectable chips ([#1134](#1134)) ([9ecda7c](9ecda7c)) * **Circle:** custom background support ([#1136](#1136)) ([bedeaa4](bedeaa4)) * **CoverHero:** new component ([#1144](#1144)) ([a655e6e](a655e6e)) * **DisplayMediaCard, PosterCard:** add extra ([#1131](#1131)) ([501cf73](501cf73)) * **EmptyState:** allow using only ButtonLink as action ([#1140](#1140)) ([d73c219](d73c219)) * **HighlightedCard:** support for alt for image ([#1135](#1135)) ([c9ba728](c9ba728)) * **Image:** Custom fallback icon in Vivo New ([#1145](#1145)) ([ec600fe](ec600fe)) * **Switch:** Improve animation ([#1142](#1142)) ([8162eed](8162eed)) * **Table:** new component ([#1129](#1129)) ([328e013](328e013)) * **Timer:** create component ([#1130](#1130)) ([0b3253e](0b3253e)) * **Touchable:** newTab support in to links ([#1143](#1143)) ([eff07e3](eff07e3))
|
🎉 This PR is included in version 15.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |

jira: WEB-1848
spec: https://www.figma.com/file/M8t65SWdTUyM44YjxuzCyu/%F0%9F%94%B8-Hero-Specs?type=design&node-id=4912%3A16384&mode=design&t=38QLKG8kvsfmgM38-1