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

wyunreal · 2023-10-09T17:13:39Z

No description provided.

github-actions · 2023-10-09T17:16:31Z

Size stats

	master	this branch	diff
Total JS	9.52 MB	9.52 MB	+1.67 kB
JS without icons	1 MB	1 MB	+1.67 kB
Lib overhead	127 kB	127 kB	0 B
Lib overhead (gzip)	32.6 kB	32.6 kB	0 B

github-actions · 2023-10-09T17:19:12Z

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

github-actions · 2023-10-09T17:19:30Z

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-oum8r33a4-tuentisre.vercel.app

Built with commit 6b93cc7.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-10-09T17:25:10Z

Screenshot tests report

✔️ All passing

atabel · 2023-10-11T07:46:16Z

src/responsive-layout.css.ts

@@ -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;


you have broken the large desktop max width:

try this snippet in master and in your branch playroom:

<ResponsiveLayout> <Placeholder/> </ResponsiveLayout>

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

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

@pladaria this !!

atabel · 2023-10-11T07:53:24Z

src/responsive-layout.css.ts

+            },
+        },
+        selectors: {
+            '& &': {


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>

ok, I'll try to fix that ;)

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

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

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

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

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

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

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

…onents

…ayouts nested

yceballost

I didn't see any weird behavior in multiple tests that I tried, LGTM.

Grande, boyscout🌝

pladaria · 2023-10-18T08:17:32Z

src/responsive-layout.css.ts

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.

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

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.

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 :(

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

atabel · 2023-10-20T07:34:15Z

src/__screenshot_tests__/responsive-layout-screenshot-test.tsx

+    {id: 'layout-responsive-layout--default', device: 'MOBILE_IOS'},
+    {id: 'layout-responsive-layout--default', device: 'MOBILE_ANDROID'},


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

ok !! fue un copy paste leftover XD

atabel · 2023-10-20T07:37:28Z

src/responsive-layout.css.ts

+                    [mq.desktop]: {
+                        margin: `0 calc(-1 * ${sideMargin})`,
+                    },


isn't this mq redundant? it's like the default case

oooh yes, this is a leftover, thanks !!

atabel · 2023-10-24T10:08:51Z

src/__screenshot_tests__/responsive-layout-screenshot-test.tsx

+    {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'},


remove this one too

# [14.28.0](v14.27.0...v14.28.0) (2023-10-25) ### Features * **PhoneNumberField:** Update formatting for Brazil ([#915](#915)) ([102466b](102466b)) * **ResponsiveLayout:** nested ResponsiveLayout background takes precedence ([#909](#909)) ([3c0fe11](3c0fe11))

tuentisre · 2023-10-25T14:05:45Z

🎉 This PR is included in version 14.28.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

now nested responsive layouts give priority to deeper instance

25a9304

wyunreal requested review from pladaria, atabel and kydorn October 9, 2023 17:13

fixed carousel spacings when in desktop

f143ed6

atabel reviewed Oct 11, 2023

View reviewed changes

Wiljan Arias Milián added 5 commits October 13, 2023 14:41

now fullwidth works inside and outside dialog

cd90d50

small refactor

19ab818

now responsive layout works inside fixed footer layout and sheet comp…

e22bb3d

…onents

fixed a bug causing everything to break when more than 2 responsive l…

bba51a2

…ayouts nested

adding some test cases

c4e2c97

wyunreal requested review from atabel and yceballost October 16, 2023 17:09

kydorn approved these changes Oct 17, 2023

View reviewed changes

yceballost approved these changes Oct 17, 2023

View reviewed changes

pladaria reviewed Oct 18, 2023

View reviewed changes

pladaria approved these changes Oct 18, 2023

View reviewed changes

Wiljan Arias Milián added 3 commits October 19, 2023 18:55

now the expand behavior can be controlled using a prop inside mistica

a524acd

reverting vars renaming

ba3fbf2

reverting sorting of imports

dae987a

wyunreal requested review from penchochris, pladaria and yceballost October 19, 2023 17:05

wyunreal changed the title ~~Now nested responsive layouts give priority to deeper instance~~ feat(ResponsiveLayout): Now nested responsive layouts give priority to deeper instance Oct 19, 2023

yceballost approved these changes Oct 19, 2023

View reviewed changes

atabel reviewed Oct 20, 2023

View reviewed changes

penchochris approved these changes Oct 20, 2023

View reviewed changes

Wiljan Arias Milián added 2 commits October 23, 2023 12:45

removing useless use case

a1f692a

removing unnedded mq

4518827

wyunreal requested a review from atabel October 23, 2023 16:10

atabel approved these changes Oct 24, 2023

View reviewed changes

pladaria approved these changes Oct 24, 2023

View reviewed changes

remove redundant test

6b93cc7

atabel changed the title ~~feat(ResponsiveLayout): Now nested responsive layouts give priority to deeper instance~~ feat(ResponsiveLayout): nested ResponsiveLayout background takes precedence Oct 25, 2023

atabel enabled auto-merge October 25, 2023 11:42

atabel added this pull request to the merge queue Oct 25, 2023

Merged via the queue into master with commit 3c0fe11 Oct 25, 2023
11 checks passed

atabel deleted the warias/responsive-layout-nested-inside-priority branch October 25, 2023 12:05

tuentisre added the released label Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wyunreal commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

atabel Oct 11, 2023

wyunreal Oct 11, 2023

atabel Oct 11, 2023

wyunreal Oct 18, 2023

atabel Oct 11, 2023

wyunreal Oct 11, 2023

wyunreal Oct 11, 2023 •

edited

Loading

atabel Oct 11, 2023

yceballost Oct 11, 2023

wyunreal Oct 16, 2023

atabel Oct 18, 2023

wyunreal Oct 18, 2023

wyunreal Oct 18, 2023

wyunreal Oct 19, 2023

yceballost left a comment

pladaria Oct 18, 2023 •

edited

Loading

atabel Oct 18, 2023

wyunreal Oct 18, 2023

wyunreal Oct 18, 2023

wyunreal Oct 19, 2023

atabel Oct 20, 2023

wyunreal Oct 23, 2023

atabel Oct 20, 2023

wyunreal Oct 23, 2023

wyunreal Oct 23, 2023

atabel Oct 24, 2023

tuentisre commented Oct 25, 2023

		{id: 'layout-responsive-layout--default', device: 'MOBILE_IOS'},
		{id: 'layout-responsive-layout--default', device: 'MOBILE_ANDROID'},

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

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

Conversation

wyunreal commented Oct 9, 2023 • edited Loading

github-actions bot commented Oct 9, 2023 • edited Loading

github-actions bot commented Oct 9, 2023 • edited Loading

github-actions bot commented Oct 9, 2023 • edited Loading

github-actions bot commented Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyunreal Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceballost left a comment

Choose a reason for hiding this comment

pladaria Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuentisre commented Oct 25, 2023

wyunreal commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023 •

edited

Loading

wyunreal Oct 11, 2023 •

edited

Loading

pladaria Oct 18, 2023 •

edited

Loading