-
-
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
Support irregular tables #456
Support irregular tables #456
Conversation
… and allow aligning their child content
This PR can't render this HTML: <table>
<tr>
<td>
<img alt='Google' src='https://www.google.com/images/branding/googlelogo/2x/googlelogo_color_92x30dp.png' />
</td>
</tr>
</table> Error: Intrinsics are not available for PlaceholderAlignment.baseline, PlaceholderAlignment.aboveBaseline, or PlaceholderAlignment.belowBaseline,
flutter doctor -v
|
@daohoangson Yes, you are right. It is easy enough to prevent that bug (by setting a |
@daohoangson Hum I see you are very aware of the whole situation and that flutter_layout_grid has problems with intrinsic heights.... you have your own html rendering flutter library? |
Yes I do. |
… is not up to par (due to the intrinsic height not being recalculated after the network image is ready to render)
@erickok I'll review this, but as soon as it is reviewed does it need to get merged in, or is it still a WIP |
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.
Just a couple of prodding questions, I didn't see anything that I would change
…rinsic height" as it breaks flowing usages of images
The problem with the images is that their intrinsic height is not being properly dispersed. This is because the cell contents are themselves |
@erickok is this PR just waiting on |
Yes, it is, if we want to support images in tables. I hope the lib maintainer can find a fix for it. |
I gave the issue here a shot:
PR created on https://github.com/vrtdev/flutter_html/tree/feature/irregular-tables. Explanation: With this implementation we can also allow the user to customize the loading widget if desired. Let me know if there are any issues with this, I didn't do super extensive testing, but it seems to work fine. |
Hey @tneotia thanks for that! I think this is very smart. I mean, ideally flutter_layout_grid gets fixed but for our use case in flutter_html it works great like this. I am going to merge it in, update the example app, test things and update my PR here. |
Fix sizing issues for images in flutter_layout_grid
Seems to be working great! @ryan-berger for me this is ready to merge now. |
Awesome! I did have a thought about this though - we might want to include a blurb about this issue in the README somewhere in case the user is using a |
@erickok Sorry, didn't see this until today. You are for sure good on this? Just double checking before I do a run through of the other library to check it out and a quick last run through of this code |
It's working well for me, yes. We use this in production too. There is no known case where the solution doesn't work, either. |
Just to leave my two cents, also using this in prod and haven't encountered any issues thus far. |
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.
Extra dependency isn't too overwhelming, makes use of better image loading, seems pretty good. LGTM
@erickok I just encountered a scenario where this doesn't work for some reason:
It's some really random and messy HTML taken from the example app for this package, and I get:
Any ideas as to why this could be happening? The above HTML renders fine on web... |
@tneotia I'll check it out. |
@erickok Although I do trust that you have been using this in production and it has worked okay, I'm now very hesitant to push this out to pub due to @tneotia 's issue. No pressure on when it needs to get done, but I just want to make sure that this is 100% good to go before pushing out into a place where it will be used in production for others. |
@ryan-berger could we do some sort of pre release with this PR? Ideally it would be nice to have some sort of beta test for this - since I found this by pure chance just messing around, it could be possible other types of HTML trigger exceptions as well. |
That's because in that HTML there are multiple |
This also fixes a crash you could get when the specs didn't match the data (cf Sub6Resources#456)
Fixed in #484 and also added proper support for the span attribute in colgroups. |
@tneotia We could definitely do some sort of beta. It looks like however @erickok has fixed the issue and I feel safe deploying this to a non-beta release after #484 is merged. It is likely that there aren't too many more edge cases, and it seems like we've got a majority of use cases covered so any edge cases we missed can be reported in a new issue. |
This fixes #242 #383 #406 #435 #213 #439 #447 #450
Adds support for irregular html tables and
colspan
/rowspan
attributes. This swaps out Flutter's Table widget for flutter_layout_grid. It still supports styling ontable
,tr
andth
/td
. Columns can be sized usingcolgroup
(percentage and fixed sizes).There is always room for improvement (for example, no row styling is applied to missing cells) but this should capture the vast majority of use cases and afaik has no regression to existing flutter_html users. I also believe this is the final thing blocking 0.11.0 users to upgrade to 1.x.