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

Improve linking support to (hopefully) all elements #2

Merged

Conversation

tneotia
Copy link

@tneotia tneotia commented Apr 24, 2021

Title.

Adds support for the elements that weren't working as noted in my review on Sub6Resources#555

Also merged branch with master.

Copy link
Member

@erickok erickok left a comment

Choose a reason for hiding this comment

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

Good work, but I think this one comment is breaking line wrapping/flowing of the elements.

@@ -434,10 +435,15 @@ class HtmlParser extends StatelessWidget {
);
} else {
///[tree] is an inline element.
return TextSpan(
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking as you are not staying in 'span-land' but are using a widget, which potentially breaks line-wrapping.

Copy link
Author

Choose a reason for hiding this comment

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

Yep I considered this. When I made this change the example app rendered fine so I decided to go ahead and push it.

This was necessary for supporting elements like <span>, <code>, <q>, etc because they don't meet any of the other criteria for other elements in the parseTree function.

The tags like <code> still displayed inline with this change. Do you know of a good way to test the inline flow? I know you worked with this type of issue with the links, so maybe a similar test process could help determine whether this is indeed an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Let me give it a try.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is indeed an issue as I suspected. This is how it should look like:
Screenshot 2021-04-29 at 10 47 06

But with your code it looks like:
Screenshot 2021-04-29 at 10 47 44

As the <span> I used (to make the background yellow) is now a separate widget instead of an inline span.

I revered this change but left your other contributions.

I used this test html:

<p>Lorem ipsum dolor amet, <span>consectetur adipiscing</span> elit, <a href="#bottom"><span>sed do eiusmod tempor</span> incididunt</a> ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</p>

@erickok erickok merged commit 4866bef into vrtdev:feature/268-inner-links Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants