-
Notifications
You must be signed in to change notification settings - Fork 7.8k
[Gallery] Preserve demo state, simplify demo width calculations on desktop #287
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
Conversation
crossAxisAlignment: CrossAxisAlignment.start, | ||
children: [ | ||
if (!isFullScreen) Expanded(child: section), | ||
SizedBox(width: !isFullScreen ? 48.0 : 0), |
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 this might be easier to read as:
if (!isFullScreen) ...[
Expanded(child: section),
SizedBox(),
]
Expanded(child: demoContent),
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 what I was saying causes the demoContent to be rebuilt from scratch. I could write it as
if (!isFullScreen) ...[
Expanded(child: section),
SizedBox(width: 48.0),
]
SizedBox(),
Expanded(child: demoContent),
and it works. It's puzzling
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.
Very strange...ok I think I prefer what you originally had instead of the 2nd sizedbox
child: GestureDetector( | ||
behavior: HitTestBehavior.opaque, | ||
onTap: () { | ||
if (_state != _DemoState.normal) { |
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.
Since the gestureDetector is added always now, does this interfere with interacting with the demo?
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 tested on chrome, iOS and macOS, and saw no adverse effects. I'll remove behavior: HitTestBehavior.opaque because it's not doing anything here and might be confusing
color: colorScheme.onSurface, | ||
fontSizeDelta: | ||
isDisplayDesktop(context) ? desktopDisplay1FontDelta : 0, | ||
return Align( |
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.
What is the Align
for?
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.
A Container within an Expanded will fill the space, even if you set width/height constraints, unless you use an Align.
TLDR: Fixes a bugs on both desktop and mobile which caused demo state to be reset.
This was caused by rendering different trees; on mobile: based on whether the options/info panels were open; on desktop, based on whether the demo was in fullscreen. Additionally on desktop, it seemed that conditionally rendering the sized box (or container) preceding the demoContent caused the demoContent widget to be fully rebuilt. Instead, I always specify its width.
This PR also removes width calculations, instead specifying a max width for info and options panes. This incidentally causes the demoContent to remain the same width regardless of the chosen section on desktop. The demoContent grows with the screen size, as before. The section on the left grows until the specified max width.
Closes material-components/material-components-flutter-gallery#312