[Reply] Cap body at 1340 for desktop and tablet #425
Conversation
There was a problem hiding this comment.
Thanks @Renzo-Olivares
Could you please take a look at the comments, and also update this branch against the latest master as I have fixed broken tests
lib/studies/reply/adaptive_nav.dart
Outdated
| defaultChild: _MailNavigator( | ||
| child: MailboxBody(key: widget.inboxKey), | ||
| ), | ||
| child: Row( |
There was a problem hiding this comment.
I think you should be able to achieve this without using a Row. Perhaps replace the Row and Flexible widgets with a Center widget?
There was a problem hiding this comment.
Thanks for the suggestion that works!
lib/studies/reply/adaptive_nav.dart
Outdated
| @@ -241,12 +241,23 @@ class _DesktopNavState extends State<_DesktopNav> | |||
| ); | |||
| }, | |||
| ), | |||
| const VerticalDivider(thickness: 1, width: 1), | |||
| const VerticalDivider(thickness: 1, width: 2), | |||
There was a problem hiding this comment.
Whats the reason for this change?
There was a problem hiding this comment.
I increased the width to remedy some artifacts (white vertical lines surrounding body shown above). From my observation, they seem to be a result of the navigation rail body not expanding to its max-width like it previously did before this change.
There was a problem hiding this comment.
Looks like these lines still show up if I don't have my browser window maximized. It looks like they show up right when I stretch the body past the 1340 width cap. Maybe because the Expanded widget is no longer taking up the entire available space, so there is a gap. I'll keep working on this.
lib/studies/reply/adaptive_nav.dart
Outdated
| child: Center( | ||
| child: ConstrainedBox( | ||
| constraints: | ||
| BoxConstraints(maxWidth: widget.extended ? 1340 : 800), |
There was a problem hiding this comment.
I think the original bug was to cap at only 1340. Why 800 for non extended?
There was a problem hiding this comment.
Yeah, you're right. I misunderstood the purpose of the cap and made it consistent for both desktop and tablet. I changed it back to match the original issue, also the margins were not constant when the width was under 1340 with the added 800 cap.
|
LGTM, assuming there are still constant margins for when the width is less than 1340 |
This reverts commit cb10d4a.
This change caps the body of the reply material study to 1340 for better text readability.
Closes #298