-
-
Notifications
You must be signed in to change notification settings - Fork 889
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
max-width attribute and auto-margins on network images for Flutter 3.10 #1359
base: master
Are you sure you want to change the base?
Conversation
@TheCarpetMerchant Do you intend to finish it or would you prefer someone to take over? |
@eEQK Would prefer someone at least look it over. I didn't even run tests because my local setup kept erroring out ; it just works for my use case as is. If what I did is enough then great, but it would still be better someone take it over, especially to separate the 2 commits which are really two different things. |
lib/src/css_box_widget.dart
Outdated
Width? maxWidthCalculated; | ||
if (style.maxWidth != null && style.width != null) { | ||
if (style.maxWidth!.unit == Unit.percent && | ||
style.width!.unit == Unit.px) { | ||
// If our max is a percentage, we want to look at the size available and not be bigger than that. | ||
try { | ||
double width = | ||
MediaQuery.of(context).size.width * (style.maxWidth!.value / 100); | ||
maxWidthCalculated = Width(width, style.width!.unit); | ||
} catch (_) {} | ||
} else if (style.width!.unit == style.maxWidth!.unit && | ||
style.width!.value > style.maxWidth!.value) { | ||
maxWidthCalculated = Width(style.maxWidth!.value, style.maxWidth!.unit); | ||
} | ||
} |
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.
Could be extracted to a method called calculateMaxWidth
and then you'd have final maxWidthCalculated = _calculateMaxWidth(style);
this way you don't have to use a var (finals should be used whenever possible)
lib/src/css_box_widget.dart
Outdated
double width = | ||
MediaQuery.of(context).size.width * (style.maxWidth!.value / 100); | ||
maxWidthCalculated = Width(width, style.width!.unit); | ||
} catch (_) {} |
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.
silent errors are generally not a good idea - you should print the exception with a stacktrace at the very least (if this code happens to fail, you won't know unless you debug carefully the whole flow)
lib/src/css_box_widget.dart
Outdated
@@ -59,8 +59,24 @@ class CssBoxWidget extends StatelessWidget { | |||
final direction = _checkTextDirection(context, textDirection); | |||
final padding = style.padding?.resolve(direction); | |||
|
|||
Width? maxWidthCalculated; | |||
if (style.maxWidth != null && style.width != null) { |
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.
if (style.maxWidth != null && style.width != null) { | |
final maxWidth = style.maxWidth; | |
final width = style.width; | |
if (maxWidth != null && width != null) { |
this way you don't have to use the bang operator below (style.width!
), just use width
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.
also, if it's not entirely clear what an if checks for, I really like to assign names to stuff I put inside if
, e.g. instead of
if (maxWidth != null && width != null) {
I would do
final hasWidthConstraint = maxWidth != null && width != null;
if(hasWidthConstraint)
but I'd say it's fine to leave the one below as it is since it's pretty self-explanatory what you're trying to do there
if (maxWidth.unit == Unit.percent && width.unit == Unit.px)
lib/src/css_box_widget.dart
Outdated
maxWidthCalculated = Width(width, style.width!.unit); | ||
} catch (_) {} | ||
} else if (style.width!.unit == style.maxWidth!.unit && | ||
style.width!.value > style.maxWidth!.value) { |
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 here would also be a good idea to name what the condition checks for
@eEQK Finally back to it. What's missing for now it the ability to compare 2 Widths that aren't in the same unit, if you know how to do that on a sufficient "best effort" basis at least that'd be great. |
Adding and using the width property in HTML() as follows: This give image width a good size but shows empty white space above and below the image. |
Any update on this issue?
unable to render image |
This PR is not ready to be merged. I made it as best as I could while trying to learn the inner workings of this beautiful package :)
CssBoxWidget
; I have no idea if this is complete or even correct.shrinkWrap
to network images so they could work with Flutter 3.10, which apparently is the cause of images going out of screen ([BUG] Images extend beyond layout boundaries in flutter_html: ^3.0.0-beta.2 #1357)Hopefully, it's not too shabby. The use of shrinkWrap should probably be removed in favor of understanding what's going wrong in the
_calculateUsedMargins
, but I haven't even tried given shrinkWrap fixes it (by bypassing the issue), at least AFAIK.