-
-
Notifications
You must be signed in to change notification settings - Fork 591
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
Conversation
@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. |
@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: 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 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 ;-). |
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.
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) { |
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.
You can't shortcut childrenNeedAView, see my comments in the main discussion.
@tomdaniel-it I am going to work on this manually, but you will still be credited for your commit. |
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
|
@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. |
Fixes #276
By changing the order of the if-else statements, custom
wrapper
properties in custom renderers get priority over the default values.