feat(ResponsiveLayout): nested ResponsiveLayout background takes precedence #909
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import {openStoryPage} from '../test-utils'; | ||
|
|
||
| const TEST_CASES = [ | ||
| {id: 'layout-responsive-layout--default', device: 'MOBILE_IOS'}, | ||
| {id: 'layout-responsive-layout--default', device: 'MOBILE_ANDROID'}, | ||
| {id: 'layout-responsive-layout--default', device: 'TABLET'}, | ||
| {id: 'layout-responsive-layout--default', device: 'DESKTOP'}, | ||
| {id: 'layout-responsive-layout--default', device: 'DESKTOP', viewport: {width: 1368, height: 770}}, | ||
| {id: 'layout-responsive-layout--nested', device: 'MOBILE_IOS'}, | ||
| {id: 'layout-responsive-layout--nested', device: 'MOBILE_ANDROID'}, | ||
|
|
||
| {id: 'layout-responsive-layout--nested', device: 'TABLET'}, | ||
| {id: 'layout-responsive-layout--nested', device: 'DESKTOP'}, | ||
| {id: 'layout-responsive-layout--nested', device: 'DESKTOP', viewport: {width: 1368, height: 770}}, | ||
| ] as const; | ||
|
|
||
| test.each(TEST_CASES)('ResponsiveLayout', async (testCase) => { | ||
| await openStoryPage({ | ||
| ...testCase, | ||
| }); | ||
|
|
||
| const image = await page.screenshot(); | ||
| expect(image).toMatchImageSnapshot(); | ||
| }); | ||
|
There was a problem hiding this comment. I'm sorry but I don't like this. These are a bunch of workarounds to allow compositions that should be forbidden. ResponsiveLayouts shouldn't be nested. This is not an issue of the component, it is a problem from the part that uses this component to compose pages or sections in unexpected ways. If I understand this PR, now you can place ResponsiveLayouts inside dialogs, sheets, footers, other ResponsiveLayouts... It makes no sense to me. Perhaps the modules shouldn't use a ResponsiveLayout and it should be added by the piece that composes those modules when needed. I guess I'm missing a lot of context about your needs and your implementation details. There was a problem hiding this comment. I agree, I don't like nested ResponsiveLayouts either, and I think we should found a way to avoid that. But this PR is not about that, we already supported nested ResponsiveLayouts but we had a bug with background color that this PR fixes. If we want to discuss a better approach for modular pages in novum webapp or other use cases where we are nesting ResponsiveLayouts, ok. But I'd keep that discussion outside of this PR There was a problem hiding this comment. Yes, its another discussion, we can talk about that. This PR gets enmarronated because the large desktop extra margins, this makes everything worse, if we had another component, or a prop to specify that we should add these extra magins, the component would be really small and simple. There was a problem hiding this comment.
This is because of the large desktop extra margins :( There was a problem hiding this comment. Implemented the props approach to fix the button fixed footer layout bug. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,60 +1,99 @@ | ||
| import {createVar, fallbackVar, style} from '@vanilla-extract/css'; | ||
| import * as mq from './media-queries.css'; | ||
| import {sprinkles} from './sprinkles.css'; | ||
| import {vars as skinVars} from './skins/skin-contract.css'; | ||
| import {vars as dialogVars} from './dialog.css'; | ||
| import {vars as sheetVars} from './sheet.css'; | ||
| import {vars as fixedFooterVars} from './fixed-footer-layout.css'; | ||
|
|
||
| export const MOBILE_SIDE_MARGIN = 16; | ||
| export const TABLET_SIDE_MARGIN = 32; | ||
| export const SMALL_DESKTOP_SIDE_MARGIN = 40; | ||
| export const LARGE_DESKTOP_MAX_WIDTH = 1224; | ||
|
There was a problem hiding this comment. you have broken the large desktop max width: try this snippet in master and in your branch playroom: <ResponsiveLayout>
<Placeholder/>
</ResponsiveLayout>There was a problem hiding this comment. Ok, I'll check, and add a test for this case, to better cover this situation. There was a problem hiding this comment. yes it would be nice to have some screenshot tests for these cases too |
||
|
|
||
| const sideMargin = createVar(); | ||
| const insideDialog = createVar(); | ||
| const notInsideDialog = createVar(); | ||
| const notInsideFixedFooter = createVar(); | ||
| export const vars = {sideMargin}; | ||
|
|
||
| export const responsiveLayoutContainer = style([ | ||
| sprinkles({width: '100%'}), | ||
| { | ||
| vars: { | ||
| [sideMargin]: '0px', | ||
| [insideDialog]: `${fallbackVar(dialogVars.insideDialog, sheetVars.insideSheetDialog, '0')}`, | ||
| [notInsideDialog]: `(1 - ${fallbackVar( | ||
| dialogVars.insideDialog, | ||
| sheetVars.insideSheetDialog, | ||
| '0' | ||
| )})`, | ||
| [notInsideFixedFooter]: `(1 - ${fallbackVar(fixedFooterVars.insideFixedFooter, '0')})`, | ||
| }, | ||
| '@media': { | ||
| [mq.desktop]: { | ||
| vars: { | ||
| [sideMargin]: `${SMALL_DESKTOP_SIDE_MARGIN}px`, | ||
| }, | ||
| }, | ||
| [mq.tablet]: { | ||
| vars: { | ||
| [sideMargin]: `${TABLET_SIDE_MARGIN}px`, | ||
| }, | ||
| }, | ||
| [mq.mobile]: { | ||
| vars: { | ||
| [sideMargin]: `${MOBILE_SIDE_MARGIN}px`, | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
| selectors: { | ||
| '& &': { | ||
|
There was a problem hiding this comment. This causes a bug, that is already broken in master: if you place a I've detected some cases in webapp, where some developers are trying to workaround it by adding their own paddings by hand 🤦 because they don't know the problem is the nested There was a problem hiding this comment. ok, I'll try to fix that ;) There was a problem hiding this comment. Can you please post here a code snippet for the playroom that reproduces the issue ? There was a problem hiding this comment. @atabel is it the same problem with sheets? There was a problem hiding this comment. I've fixed using couple css variables, I don't know if there is any better option, any advice welcome. There was a problem hiding this comment. hmmm... I think the css variables approach is too convoluted. I'd prefer a prop in There was a problem hiding this comment. I would go for a different component, for internal responsive layout, becuase it wont needs to handle large desktop extra margins, for example (that makes everything lot complex). |
||
| width: 'auto', | ||
| margin: `0 calc(-1 * ${notInsideFixedFooter} * ${sideMargin})`, | ||
| '@media': { | ||
| [mq.desktop]: { | ||
| margin: `0 calc(-1 * ${sideMargin})`, | ||
| }, | ||
|
There was a problem hiding this comment. isn't this mq redundant? it's like the default case There was a problem hiding this comment. oooh yes, this is a leftover, thanks !! |
||
| [mq.largeDesktop]: { | ||
| margin: `0 calc(-1 * ( | ||
| ${notInsideDialog} * (100vw - ${LARGE_DESKTOP_MAX_WIDTH}px) / 2 + | ||
| ${insideDialog} * ${SMALL_DESKTOP_SIDE_MARGIN}px | ||
| ))`, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ]); | ||
|
|
||
| export const fullwidthContainer = style([ | ||
| { | ||
| vars: { | ||
| [sideMargin]: '0px', | ||
| }, | ||
| }, | ||
| ]); | ||
|
|
||
| export const backgroundVariant = { | ||
| inverse: sprinkles({background: skinVars.colors.backgroundBrand}), | ||
| alternative: sprinkles({background: skinVars.colors.backgroundAlternative}), | ||
| }; | ||
|
|
||
| export const responsiveLayout = style({ | ||
| paddingLeft: 'env(safe-area-inset-left)', | ||
| paddingRight: 'env(safe-area-inset-right)', | ||
|
|
||
| margin: `0 ${sideMargin}`, | ||
| '@media': { | ||
| [mq.largeDesktop]: { | ||
| margin: `0 calc( | ||
| ${notInsideDialog} * (100vw - ${LARGE_DESKTOP_MAX_WIDTH}px) / 2 + | ||
| ${insideDialog} * ${SMALL_DESKTOP_SIDE_MARGIN}px | ||
| )`, | ||
| width: `calc(${notInsideDialog} * ${LARGE_DESKTOP_MAX_WIDTH}px)`, | ||
| minWidth: `calc(${insideDialog} * (100% - ${SMALL_DESKTOP_SIDE_MARGIN * 2}px))`, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
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'd remove one of these. Testing iOS/Android doesn't give any value in this component, the relevant thing here is the screen size. Having different tests for Android/iOS makes sense in components that change betwen platforms, like the Switch
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.
ok !! fue un copy paste leftover XD