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

Prefer custom wrapper of custom renderer over default #277

Closed
wants to merge 1 commit into from

Conversation

tomdaniel-it
Copy link

Fixes #276

By changing the order of the if-else statements, custom wrapper properties in custom renderers get priority over the default values.

@jsamr jsamr added is:looking good Ready to merge. pr:fix Any code submission that repairs something broken. labels Jul 5, 2020
@jsamr jsamr added pr:review needed This PR requires review from maintainers. and removed is:looking good Ready to merge. labels Jul 13, 2020
@jsamr
Copy link
Collaborator

jsamr commented Jul 16, 2020

@BojanRatkovic @tomdaniel-it I would like some tests implemented for this feature, and I want to make sure it doesn't break any present behavior prior to getting merged. Could you provide a test plan? I don't mind implementing the tests, I'd just like to see a few concrete snippets and a description with the expected outcome.

@jsamr
Copy link
Collaborator

jsamr commented Jul 18, 2020

@tomdaniel-it I implemented a regression test in test/276, and good news: your code fixes the issue. However, a test I implemented earlier failed:

2020-07-18-185832_757x340_scrot

This test fails because you should not override the behavior of checking for children needing views. In case a custom renderer has a Text wrapper, and a children which requires View (such as <img>), we end up with a View nested inside a Text, which is not supported in some yet recent React Native versions. This test should remains the first, after which the custom renderers ... etc.

To get those tests, you need to rebase your work on this test/276 branch. If you want help to do so, send me an email ;-).

Copy link
Collaborator

@jsamr jsamr left a comment

Choose a reason for hiding this comment

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

Pass the tests and this PR should be good to be merged.

@@ -300,15 +300,16 @@ export default class HTML extends PureComponent {
// Recursively map all children with this method
children = this.associateRawTexts(this.mapDOMNodesTORNElements(children, name));
}
if (this.childrenNeedAView(children) || BLOCK_TAGS.indexOf(name.toLowerCase()) !== -1) {
if (this.renderers[name] && this.renderers[name].wrapper) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't shortcut childrenNeedAView, see my comments in the main discussion.

@jsamr jsamr added is:waiting for feedback Waiting for OP input. and removed pr:review needed This PR requires review from maintainers. labels Jul 18, 2020
@jsamr
Copy link
Collaborator

jsamr commented Jul 23, 2020

@tomdaniel-it I am going to work on this manually, but you will still be credited for your commit.

@jsamr
Copy link
Collaborator

jsamr commented Jul 23, 2020

This is my solution of top of your's (see 245be04):

diff --git a/src/HTML.js b/src/HTML.js
index bb10f39..a58b9cc 100644
--- a/src/HTML.js
+++ b/src/HTML.js
@@ -296,18 +296,17 @@ export default class HTML extends PureComponent {
                     // Recursively map all children with this method
                     children = this.associateRawTexts(this.mapDOMNodesTORNElements(children, name));
                 }
-                if (this.renderers[name] && this.renderers[name].wrapper) {
-                    // Prefer custom wrapper if present in renderer
-                    return { wrapper: this.renderers[name].wrapper, children, attribs, parent, tagName: name, parentTag };
-                } else if (this.childrenNeedAView(children) || BLOCK_TAGS.indexOf(name.toLowerCase()) !== -1) {
-                    // If children cannot be nested in a Text, or if the tag
-                    // maps to a block element, use a view
-                    return { wrapper: 'View', children, attribs, parent, tagName: name, parentTag };
+                let wrapper = "View";
+                if (this.childrenNeedAView(children)) {
+                    wrapper = "View";
+                } else if (this.renderers[name] && this.renderers[name].wrapper) {
+                    wrapper = this.renderers[name].wrapper;
+                } else if (BLOCK_TAGS.indexOf(name.toLowerCase()) !== -1) {
+                    wrapper = "View";
                 } else if (TEXT_TAGS.indexOf(name.toLowerCase()) !== -1 || MIXED_TAGS.indexOf(name.toLowerCase()) !== -1) {
-                    // We are able to nest its children inside a Text
-                    return { wrapper: 'Text', children, attribs, parent, tagName: name, parentTag };
+                    wrapper = "Text";
                 }
-                return { wrapper: 'View', children, attribs, parent, tagName: name, parentTag };
+                return { wrapper, children, attribs, parent, tagName: name, parentTag };
             }
         })
         .filter((parsedNode) => parsedNode !== false && parsedNode !== undefined) // remove useless nodes

@jsamr
Copy link
Collaborator

jsamr commented Jul 23, 2020

@tomdaniel-it Thanks again for pointing out this issue and offering a solution! Your commit has been a bit reshaped to pass the tests and merged to master manually, see 245be04.

@jsamr jsamr closed this Jul 23, 2020
@jsamr jsamr removed the is:waiting for feedback Waiting for OP input. label Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix Any code submission that repairs something broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom renderer wrapper property being ignored
3 participants