-
Notifications
You must be signed in to change notification settings - Fork 11
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(CoverHero): new component #1144
Conversation
Size stats
|
Accessibility report ℹ️ You can run this locally by executing |
Deploy preview for mistica-web ready! ✅ Preview Built with commit d465585. |
@@ -27,7 +27,6 @@ export const image = style([ | |||
borderRadius: skinVars.borderRadii.container, | |||
}), | |||
{ | |||
zIndex: 1, |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :(
@@ -282,11 +282,14 @@ const CardActionPauseIcon = ({color}: IconProps) => <IconPauseFilled color={colo | |||
|
|||
const CardActionPlayIcon = ({color}: IconProps) => <IconPlayFilled color={color} size={12} />; | |||
|
|||
const useVideoWithControls = ( | |||
export const useVideoWithControls = ( |
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.
should we abstract this in a separate file instead? It's not something specific to cards anymore
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.
Yes I thought about it, I can extract it if you prefer
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.
Yes, I think we should do it in order to keep the code well organized
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
talked offline, we'll move this refactor to the future (when we have over media variant)
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.
The lenght of the description changes in desktop when there's a side slot depending if centered is true|false, the expected lenght should be the same is if was left aligned.
src/cover-hero.tsx
Outdated
<Box paddingY={noPaddingY ? 0 : {desktop: 56, tablet: 56, mobile: 24}}> | ||
<Stack space={24}> | ||
{centered && !sideExtra ? ( | ||
mainContent |
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.
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.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to document the intention, but I can remove it
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've rewritten the comment with a better explanation and added a reference to this PR: ComponentDriven/csf#76
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.
🙌
# [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