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.

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

New option: shrinkToFit #148

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Conversation

amake
Copy link
Contributor

@amake amake commented Aug 26, 2019

This PR adds a new shrinkToFit option to Html. When true, the widget takes up only its natural width, rather than attempt to take up the full width of the screen.

Rationale

Flutter lacks a built-in way to style text without breaking it up into runs. For instance

Hello, world!

must be created as two separate runs of Hello, and world!. This is a big problem for localization, because it's not possible to meaningfully translate the phrase "Hello, world!" when it is broken up like this.

Thus for real-world apps targeting multiple languages, a way to style a single string is essential. flutter_html nearly fits the bill, except for one key issue: it always tries to take up the full width of the screen. This makes it impossible to lay out e.g. a row of labels: Row(children: [Html(foo), Html(bar), Html(baz)]) because any given flex arrangement will not work when the content length has changed in translation.

Thus the solution is to allow the Html widget to take up only its natural width.

API

The shrinkToFit name I took from the ListView option, as it seemed fitting. It is false by default, so current users should see no change.

This PR drops the width: parameter of the HtmlRichTextParser class, which was unused anyway. Let me know if you would prefer to keep it for backwards compatibility.

Other changes

The leading char in li is now a TextSpan with the li content as its child. This wasn't strictly necessary, but simplifies the structure nicely and is actually a visual improvement: the leading char was previously not flush with the child baseline in the case of differing text heights, but now it is. Note that the code as written will conflict with #131, but it is easily reconciled.

@amake
Copy link
Contributor Author

amake commented Aug 26, 2019

I now realize this is basically a different approach to solving the issues in #83 and #75, but I feel my solution is better: setting the alignment within Html does not solve use cases where multiple Htmls are laid out in a row; instead Html should simply not expand more than it needs, and control over alignment should be left to the user.

@Sub6Resources Sub6Resources self-requested a review August 27, 2019 14:11
@Sub6Resources Sub6Resources self-assigned this Aug 27, 2019
@Sub6Resources Sub6Resources added enhancement New feature or request high-priority labels Aug 27, 2019
@Sub6Resources Sub6Resources added this to the 0.11.0 milestone Aug 29, 2019
@Sub6Resources
Copy link
Owner

Thank you for the detailed pull request, this is very helpful. I think I prefer this approach rather than the approach proposed in #83. My initial glance at the code changes look good to me, but I'd like to run this through a more visual test with a few different complex html samples I have, so review will probably take a few days. If everything goes well, this should be merged and released with version 0.11.0 in a week or two.

@Sub6Resources Sub6Resources changed the base branch from master to 0.11.0 September 10, 2019 04:33
Copy link
Owner

@Sub6Resources Sub6Resources left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants