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.

Sign up for GitHub

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

Jump to bottom

feat: add support for children transclude #36

Closed
wants to merge 1 commit into from
Closed

feat: add support for children transclude #36

wants to merge 1 commit into from

Conversation

spirosikmd
Copy link

Fixes #12

@bcherny I followed the recommendations mentioned in the issue. I created a prototype. Seems that it is possible. However, the last part I am not sure how to proceed best. Current status is:

<test-angular-two foo="foo" bar="bar" baz="baz" class="ng-scope ng-isolate-scope">
  <div data-reactroot="">
    <p><!-- react-text: 3 -->Foo: <!-- /react-text --><!-- react-text: 4 -->1<!-- /react-text --></p>
    <p><!-- react-text: 6 -->Bar: <!-- /react-text --><!-- react-text: 7 -->true,false<!-- /react-text --></p>
    <p>Baz</p>
    <!-- react-text: 9 -->&lt;span className="ng-scope"&gt;Transcluded&lt;/span&gt;<!-- /react-text -->
    <!-- react-text: 10 -->&lt;p className="ng-scope"&gt;Transcluded&lt;/p&gt;<!-- /react-text -->
  </div>
</test-angular-two>

the transcluded components are added as children, but they are escaped. Any idea? Maybe use angular $sce?

@spirosikmd spirosikmd force-pushed the support-transclude branch 4 times, most recently from b45e84e to 42bc4dc Compare September 6, 2017 14:35
@spirosikmd
Copy link
Author

spirosikmd commented Sep 6, 2017 •
edited
Loading

So @bcherny I managed to fix the escaping issue by using html-react-parser! I think this is ready for review. Let me know what you think?

@bcherny bcherny closed this Sep 6, 2017
@bcherny bcherny reopened this Sep 6, 2017
@bcherny
Copy link
Contributor

bcherny commented Sep 6, 2017

@spirosikmd This is excellent, thanks for that hard work! A couple of notes:

  • Can you please add tests for updating the scope when transcluded elements use stuff from that scope (making sure the transcluded content updates, isn't double-rendered, etc.)
  • How much do the extra dependencies add to the build size?

@spirosikmd
Copy link
Author

Hi @bcherny! Thank you for providing the package in the first place!

The sizes for support-transclude branch are:

577B index.d.ts
2.0K index.es2015.js
2.3K index.js
1.5K index.js.map

compared to master branch:

577B index.d.ts
1.4K index.es2015.js
2.4K index.js
889B index.js.map

Size of index.js hasn't increased at all. However, index.es2015.js size increased by ~0.6K. Do you think this is would be a problem?

@bcherny
Copy link
Contributor

bcherny commented Sep 6, 2017

@spirosikmd 0.6kb is no big deal, but what happens when you run that through browserify (to pull in htmltojsx, html-react-parser, and all of their dependencies)?

@spirosikmd
Copy link
Author

When running the following command

./node_modules/.bin/browserify index.js -o bundle.js

on both branches, the bundle.js file ends up being 2.5M for support-transclude and 1.1M for master. Big difference I think. How to improve that?

@bcherny
Copy link
Contributor

bcherny commented Sep 6, 2017 •
edited
Loading

I was worried it would be more in the 10-20mb range. 2.5mb is fine I think. A tree-shaking compiler (rollup, webpack) will improve that size, and we get a lot of functionality for that extra 1.4mb.

@spirosikmd
Copy link
Author

Cool ok then! I will work on adding some tests and docs for this feature.

@spirosikmd spirosikmd force-pushed the support-transclude branch 2 times, most recently from 67a0ce1 to 506ab0e Compare September 7, 2017 14:44
spirosikmd
spirosikmd commented Sep 7, 2017
View reviewed changes
test.tsx
$compile(element)(scope)
$rootScope.$apply()
expect(element.find('span').text()).toBe('1')
// scope.$apply(() =>
Copy link
Author

Choose a reason for hiding this comment

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

@bcherny we can now transclude other components as well, and bind once with initial rendering. However, I cannot manage to update the two way binding. Any idea? Would it be possible to release without this support for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

@spirosikmd This looks like a pretty serious bug to me. In your test it's not that the view doesn't update, it's that it reverts to its original untemplated version (eg. in this case it's {'{'}{'{'}foo{'}'}{'}'}. If that's correct, I would argue this should be fixed before we merge. Broken support for transclusion (that sets false expectations) could be worse than no support.

Copy link
Author

Choose a reason for hiding this comment

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

You are definitely right. This should be fixed first!

@bcherny
Copy link
Contributor

bcherny commented Oct 22, 2017

@spirosikmd Any updates on this?

@spirosikmd
Copy link
Author

Hey @bcherny! I haven't worked on this yet. I am trying the approach described in this article, passing all the data needed as props and don't use transclusion.

@bcherny
Copy link
Contributor

bcherny commented Oct 24, 2017

@spirosikmd Sounds good! I was doing some PR cleanup and wanted to make sure this wasn't forgotten.

@andreoav
Copy link

andreoav commented Mar 5, 2018

Any update?

@gregbty gregbty mentioned this pull request Mar 27, 2018
Closed
@gairon
Copy link

gairon commented Dec 29, 2018

Any update?

@spirosikmd
Copy link
Author

Hi @andreoav @gairon! I stopped working on this. I won't invest in angular.js anymore. I see @gregbty is working on a similar PR. He runs into the same issues I was running as well.

@spirosikmd spirosikmd closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcherny bcherny bcherny left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for children/transclusion
4 participants
@spirosikmd @bcherny @andreoav @gairon