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

Upgrade customRender to imitate customImageRender & add support for customRender for SelectableHtml #632

Merged
merged 18 commits into from
Dec 16, 2021

Conversation

tneotia
Copy link
Collaborator

@tneotia tneotia commented Apr 25, 2021

Title.

The API is extremely similar to that of custom image render. A brief summary:

  1. Users provide a Map<CustomRenderMatcher, CustomRender> in their widget code.

  2. CustomRenderMatcher passes RenderContext to the user and requires a bool to be returned.

  3. CustomRender has two constructors: CustomRender.fromInlineSpan and CustomRender.fromWidget. Users provide one or the other in the above Map. Both constructors pass the RenderContext and a function that returns the built children when called. This avoids unncecessary widget creation if the user doesn't need to display children.

  4. The package has a number of default renders (see custom_render.dart) and prioritizes the user renders.

  5. For SelectableHtml, users pass Map<CustomRenderMatcher, SelectableCustomRender> in their widget code.

  6. SelectableCustomRender extends CustomRender and allows users to pass a TextSpan. It has the same constructor format as CustomRender.

If you have any thoughts on the API let me know.

A couple of things to still tackle:

  1. I disabled some tests due to some modifications I needed to make to the _lexDomTree function. We have to figure out a way to get those working again. Done.

  2. Update README Done.

  3. Possibly add some easy customization parameters to the default renders? At the moment there are none. Done - now you can do stuff like this:

Copy link
Collaborator

@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.

I think you did you great job here and it is exactly how I imagined the evolution of the API to go. This also makes modularization easy.

Of course it requires some extensive regression testing. I am sorry I don't have so much time now...

@tneotia
Copy link
Collaborator Author

tneotia commented Apr 29, 2021

No problem at all. I don't create these PRs looking for immediate feedback, everyone has their own obligations to attend to first, even I was a bit inactive for a few weeks :)

In the meantime I'll see about adding customization parameters to the default ones and updating docs.

@tneotia tneotia mentioned this pull request May 9, 2021
6 tasks
…into feature/upgrade-custom-render

� Conflicts:
�	README.md
�	example/lib/main.dart
�	lib/html_parser.dart
…into feature/upgrade-custom-render

� Conflicts:
�	lib/flutter_html.dart
�	lib/html_parser.dart
…into feature/upgrade-custom-render

� Conflicts:
�	lib/flutter_html.dart
�	lib/html_parser.dart
@tneotia tneotia changed the title Upgrade customRender to imitate customImageRender Upgrade customRender to imitate customImageRender & add support for customRender for SelectableHtml May 31, 2021
@tneotia
Copy link
Collaborator Author

tneotia commented May 31, 2021

@erickok I know you're busy but since we've been making so many changes to the parser and the API recently it's been a bit hard to keep fixing merge conflicts with this branch - I always feel like I've missed something when migrating the parser changes to the customRender API. If you have any time to review and potentially merge that would be super helpful, but no pressure. I totally understand!

@erickok
Copy link
Collaborator

erickok commented Jun 1, 2021

I very much understand. I also find this an important topic. That's why I try to close as many fix PRs and small features as possible. The reworking means a sizable api change and as such warrants another major version bump (even though the internals largely remain the same). This means we have to try to get 2.0 stable asap so this rework can come, which aids modularisation. I think we're close though and much appreciate your work. Almost there. Also, in one month time I will have much more time myself to get back on the library, so it won't all be on you any more.

…ustom-render

# Conflicts:
#	lib/html_parser.dart
#	test/html_parser_test.dart
@erickok
Copy link
Collaborator

erickok commented Jul 28, 2021

This is great work @tneotia .

I wanted to make some small suggestions but it seems I actually pushed directly to your repo. 🤦 For which I apparently have rights. Anyway check out tneotia@1c7b9e2 for that.

@tneotia
Copy link
Collaborator Author

tneotia commented Jul 28, 2021

No problem, thanks for fixing the conflicts! Otherwise your changes are fine with me :)

@erickok
Copy link
Collaborator

erickok commented Jul 28, 2021

What do you think @tneotia of - since we are breaking anyway - removing the buildChildren parameter such that we don't have to build the child tree eagerly? In most cases a custom render would not use them anyway. Of course we can/should then provide a (convenience) method to render the children using the default rendering engine in case you do want them. In fact you might want to have a convenience method to render the tag as default itself, only to wrap it with some custom code.

@tneotia
Copy link
Collaborator Author

tneotia commented Jul 28, 2021

@erickok I might be misunderstanding what you mean but I think in the new implementation we don't build the child tree eagerly - buildChildren is a function, that when invoked, will build the child tree. We don't explicitly build the tree unless the user wants us to do so by using buildChildren() in their code. (See https://github.com/tneotia/flutter_html/blob/feature/upgrade-custom-render/lib/html_parser.dart#L335)

As for the convenience method to use the default rendering engine, it would be a little difficult imho because the default renders return InlineSpans which can't be wrapped by regular widgets. We would have to make a separate implementation for each of our default renders since they are all structured differently. This could maybe be a feature we add in the future, though.

…into feature/upgrade-custom-render

� Conflicts:
�	README.md
�	lib/flutter_html.dart
�	lib/html_parser.dart
@tneotia
Copy link
Collaborator Author

tneotia commented Dec 10, 2021

@erickok this has been updated to reflect latest changes on master as well.

I'd say if we want to merge #922, #932, and #879, we may as well go all-in and finish the modularization process to avoid multiple breaking change releases in a row. The process would look like this:

This way, we update all breaking changes in (basically) one release, rather than making major version updates for each breaking change. That might be a lot of changes in one release but I think it should be fine, mostly the code is just refactored to different places and I don't expect any functionality to break.

If this sounds good to you I can make a v3.0.0 migration guide as well.

@erickok erickok merged commit b5aa467 into Sub6Resources:master Dec 16, 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.

None yet

2 participants