Skip to content
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.

Sign up for GitHub

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(ResponsiveLayout): nested ResponsiveLayout background takes precedence #909

Merged
merged 13 commits into from
Oct 25, 2023
Merged

feat(ResponsiveLayout): nested ResponsiveLayout background takes precedence #909

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
25a9304
now nested responsive layouts give priority to deeper instance
Oct 9, 2023
f143ed6
fixed carousel spacings when in desktop
Oct 10, 2023
cd90d50
now fullwidth works inside and outside dialog
Oct 13, 2023
19ab818
small refactor
Oct 13, 2023
e22bb3d
now responsive layout works inside fixed footer layout and sheet comp…
Oct 13, 2023
bba51a2
fixed a bug causing everything to break when more than 2 responsive l…
Oct 13, 2023
c4e2c97
adding some test cases
Oct 16, 2023
a524acd
now the expand behavior can be controlled using a prop inside mistica
Oct 19, 2023
ba3fbf2
reverting vars renaming
Oct 19, 2023
dae987a
reverting sorting of imports
Oct 19, 2023
a1f692a
removing useless use case
Oct 23, 2023
4518827
removing unnedded mq
Oct 23, 2023
6b93cc7
remove redundant test
atabel Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/carousel.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,12 @@ export const carousel = style([
const responsiveLayoutSideMargin = fallbackVar(responsiveLayoutVars.sideMargin, '0px');

export const carouselWithScroll = style({
margin: `0 calc(${responsiveLayoutSideMargin} * -1)`,
margin: 0,
'@media': {
[mq.tabletOrSmaller]: {
margin: `0 calc(${responsiveLayoutSideMargin} * -1)`,
},
},
});

export const centeredCarousel = style({
Expand Down
9 changes: 3 additions & 6 deletions src/hero.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import {createVar, style} from '@vanilla-extract/css';
import * as mq from './media-queries.css';
import {sprinkles} from './sprinkles.css';
import {
LARGE_DESKTOP_MAX_WIDTH,
MOBILE_SIDE_MARGIN,
SMALL_DESKTOP_SIDE_MARGIN,
TABLET_SIDE_MARGIN,
} from './responsive-layout.css';
import {MOBILE_SIDE_MARGIN, SMALL_DESKTOP_SIDE_MARGIN, TABLET_SIDE_MARGIN} from './responsive-layout.css';

const LARGE_DESKTOP_MAX_WIDTH = 1224;

const height = createVar();

Expand Down
69 changes: 32 additions & 37 deletions src/responsive-layout.css.ts
View file
Open in desktop
Copy link
Member

@pladaria pladaria Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this PR, now you can place ResponsiveLayouts inside dialogs, sheets, footers, other ResponsiveLayouts... It makes no sense to me.

This is because of the large desktop extra margins :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the props approach to fix the button fixed footer layout bug.

Original file line number Diff line number Diff line change
Expand Up @@ -6,57 +6,52 @@ import {vars as skinVars} from './skins/skin-contract.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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have broken the large desktop max width:

try this snippet in master and in your branch playroom:

<ResponsiveLayout>
  <Placeholder/>
</ResponsiveLayout>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll check, and add a test for this case, to better cover this situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it would be nice to have some screenshot tests for these cases too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pladaria this !!


const sideMargin = createVar();
export const vars = {sideMargin};

export const container = sprinkles({width: '100%'});
export const container = style([
sprinkles({width: '100%'}),
{
vars: {
[sideMargin]: '0px',
},
'@media': {
[mq.desktopOrBigger]: {
vars: {
[sideMargin]: `${SMALL_DESKTOP_SIDE_MARGIN}px`,
},
},
[mq.tablet]: {
vars: {
[sideMargin]: `${TABLET_SIDE_MARGIN}px`,
},
},
[mq.mobile]: {
vars: {
[sideMargin]: `${MOBILE_SIDE_MARGIN}px`,
},
},
},
selectors: {
'& &': {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes a bug, that is already broken in master: if you place a <ButtonFixedFooterLayout> inside a <ResponsiveLayout> the footer paddings are broken because the internal <ResponsiveLayout> stops working.

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 <ResponsiveLayout>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll try to fix that ;)

Copy link
Contributor Author

@wyunreal wyunreal Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please post here a code snippet for the playroom that reproduces the issue ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://mistica-web.vercel.app/playroom#?code=N4Igxg9gJgpiBcIA8AlGBnADhAduglgG4wAyAhgJ4QCuALgHwA6OABC0gEJ224Bi%2BADxhReECLRgAnclTrM2bAEbdcAXmCcVOAAqT8AWzKSKLXLozp1ACgCULVfRbAAvs-oBBMLXy4kAei5aHh09Q2N6Z3kFdBhIHCgjCkDg9SiFdmTcAGVY3ATjUxCLazsHJ1cPLx8cf0ycHLj8iiZWBUjWlvSsiH0YFjiJHFoWAAspGCjarX4hETEJaUoaBmZ-NCxcAmIZZZaQABoQWjHe9AQAbRAOABsyahAAXUOAd3woY7P4c4BmADYABn2AEY-gAOB7OIA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atabel is it the same problem with sheets?
https://tinyurl.com/yqdcvsuf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed using couple css variables, I don't know if there is any better option, any advice welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... I think the css variables approach is too convoluted. I'd prefer a prop in <ResponsiveLayout> to disable the special nesting behavior in the cases we know we don't want it (fixedFooter, dialogs, etc). If we don't want to make that prop public we could have an InternalResponsiveLayout that accepts that prop, ResponsiveLayout would just be a wrapper of InternalResponsiveLayout without the additional prop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was muy preferred solution, but I tried to keep the component's interface unchanged. so What do you prefer @atabel , @pladaria ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done !

margin: `0 calc(-1 * ${sideMargin})`,
width: 'auto',
},
},
},
]);

export const backgroundVariant = {
inverse: sprinkles({background: skinVars.colors.backgroundBrand}),
alternative: sprinkles({background: skinVars.colors.backgroundAlternative}),
};

export const responsiveLayout = style({
margin: 'auto',
paddingLeft: 'env(safe-area-inset-left)',
paddingRight: 'env(safe-area-inset-right)',

vars: {
[sideMargin]: '0px',
},

'@media': {
[mq.largeDesktop]: {
width: LARGE_DESKTOP_MAX_WIDTH,
maxWidth: `calc(100% - ${SMALL_DESKTOP_SIDE_MARGIN * 2}px)`, // to make ResponsiveLayout work inside desktop modals
},
[mq.desktop]: {
margin: `0 ${SMALL_DESKTOP_SIDE_MARGIN}px`,
},
[mq.tablet]: {
margin: `0 ${TABLET_SIDE_MARGIN}px`,

vars: {
[sideMargin]: `${TABLET_SIDE_MARGIN}px`,
},
},
[mq.mobile]: {
margin: `0 ${MOBILE_SIDE_MARGIN}px`,

vars: {
[sideMargin]: `${MOBILE_SIDE_MARGIN}px`,
},
},
},

selectors: {
'& &': {
margin: 0,
width: 'auto',
},
},
margin: `0 ${sideMargin}`,
});

export const fullWidth = style([
Expand Down