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

Addon-docs/Angular: Inline rendering support with angular-elements #13525

Merged
merged 5 commits into from
Jan 28, 2021

Conversation

ThibaudAV
Copy link
Contributor

@ThibaudAV ThibaudAV commented Dec 27, 2020

Issue: #8153 #12264 #13480

Another PR : #11339

TODO / Question :

  • Check the optional dependencies on angular element works correctly on a project that is not in monorepo.
  • Add / correct the doc
  • Optimization with change of props (avoids refresh the whole component if props change) (if necessary in another PR)
  • Add a more complete example in angular-cli?
  • Should we activate inlineStories by default?
  • Add tests

What I did

Uses angular element to transform story component into customElements (web-component).
The custom element is then added in React.

Add optional @storybook/angular dependency to @storybook/addon-docs.
Add optional @angular/elements & @webcomponents/custom-elements dependencies in @storybook/angular.
Only one file (ElementRendererService ) uses these dependencies, so if it is not called these depeandace still optional. It seems to me 🤔
:not-sure:

User has to specify these dependencies if he uses prepareForInline

I tried to add them in the addon-docs but it adds more angular dependency. I figure it's better if they're all in @storybook/angular?
What do you think about it?

How it work

Add : yarn add -D @angular/elements @webcomponents/custom-elements
Then update .storybook/preview.js:

import { addParameters } from '@storybook/angular';
import { prepareForInline } from '@storybook/addon-docs/angular/inline';
addParameters({
  docs: {
    inlineStories: true,
    prepareForInline,
  },
});

A special rendering is made by prepareForInline:

  • is called for each story
  • prepareForInline use ElementRendererService from @storybook/angular
  • ElementRendererService use singleton instance of angular platform from RendererService
  • ElementRendererService generate custom ElementsModule from storyInput ("main component" is generated in the same way as the "classic render")
  • ElementsModule is bootstrap by angular in <div id="root">
  • ElementsModule use createCustomElement from @angular/elements to extract the main component (the one of the story) and return them to prepareForInline as customElement
  • prepareForInline define in customElements new customElement

Note: as it is not possible to delete defined customElements. An "increment" allows to make each new generation unique.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This is great and will make a lot of people happy. I don't understand enough about the Angular/Web components story to comment on the high-level approach, but I believe @stupidawesome suggested this in a previous conversation with @kroeder and me back when we were discussing Ivy rendering.

@kroeder @stupidawesome @costalvaro @petermikitsh @manuelmeister can you please take a look at this PR and give feedback?

Copy link
Member

@kroeder kroeder left a comment

Choose a reason for hiding this comment

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

Nice work so far 🙂

Your plan is to solve the issues with Storybooks docs mode, right?

I tried to add them in the addon-docs but it adds more angular dependency. I figure it's better if they're all in @storybook/angular?
What do you think about it?

@shilman Something you might be able to give feedback to.

@ThibaudAV
Copy link
Contributor Author

thanks :)

Your plan is to solve the issues with Storybooks docs mode, right?

yes to propose an "inline" alternative to avoid iframe (adapts to the size of the component, changes in line with docs. can change theme with toolbar dynamically, ...)

@shilman
Copy link
Member

shilman commented Jan 4, 2021

cc @mandarini

@ThibaudAV ThibaudAV force-pushed the angular-inline-docs branch from bac1218 to 95b4524 Compare January 14, 2021 18:05
@ThibaudAV ThibaudAV force-pushed the angular-inline-docs branch 3 times, most recently from 2f3b47b to 81a9c4e Compare January 15, 2021 19:36
@ThibaudAV
Copy link
Contributor Author

ThibaudAV commented Jan 15, 2021

@ThibaudAV ThibaudAV force-pushed the angular-inline-docs branch 5 times, most recently from 39aab49 to 24be443 Compare January 21, 2021 17:26
@ThibaudAV
Copy link
Contributor Author

I'm not sure how to test. :/
if you have ideas, I'm interested :)
I figure it can be done with chromatic when / if this feature is used by default. 🤷‍♂️

@ThibaudAV ThibaudAV marked this pull request as ready for review January 22, 2021 13:35
@ThibaudAV ThibaudAV requested review from shilman, kroeder and a team January 22, 2021 13:35
@gaetanmaisse gaetanmaisse self-assigned this Jan 25, 2021
@shilman shilman changed the title Angular: inline rendering support in addon-docs with angular-elements Addon-docs/Angular: Inline rendering support with angular-elements Jan 26, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Working great! One small change.

@@ -1,9 +1,8 @@
import { DocButtonComponent } from './doc-button.component';

export default {
title: 'DocButton',
title: 'Addons/Docs/DocButton',
Copy link
Member

Choose a reason for hiding this comment

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

All the other angular-cli examples are in Addon/x. I think "addons" is better, but I suggest making the switch in a separate PR and updating all the other examples at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaa you have unmasked me
I would like to rework all stories example. group them, sort them,..., in order to see quickly their goals
I will do it in another PR

@ThibaudAV
Copy link
Contributor Author

I do a final manual test :
-> New project with storyshots addon and without angular element

And then it will seem good to me

@shilman
Copy link
Member

shilman commented Jan 28, 2021

Let me know when you're ready. Super excited about this!!!

@ThibaudAV ThibaudAV force-pushed the angular-inline-docs branch 2 times, most recently from 1b5722c to 226564a Compare January 28, 2021 16:58
@ThibaudAV
Copy link
Contributor Author

@shilman My test was useful and I had to make a change :

add a specific file so that the whole code branch (with element import) is not used if the user does not use prepareForInline
94230a2#diff-61c1e0294b5dcd012e60f3b1aa8cb382baee5a19876bd82b22394716b90a4e46R1

@ThibaudAV ThibaudAV force-pushed the angular-inline-docs branch from 226564a to 53248ca Compare January 28, 2021 17:20
@shilman shilman added the run e2e extended test suite Run the e2e extended test suite in CircleCI workflows label Jan 28, 2021
@shilman
Copy link
Member

shilman commented Jan 28, 2021

@ThibaudAV are we good to go now after the tests pass?

@ThibaudAV
Copy link
Contributor Author

yes it's ok for me

@shilman
Copy link
Member

shilman commented Jan 28, 2021

There's an e2e failure but it's related to a different PR that I merged and @phated is following up on that. So I'm merging this since everything else looks great. ❤️

@danielleroux
Copy link

danielleroux commented Feb 23, 2021

@ThibaudAV Hi iam testing the new inline feature in our component library (Angular 10) but iam discovered a strange behavior in my project. I have to compile my library with the ngcc compiler after each build, otherwise i get the following error message:

zone.js:690 Unhandled Promise rejection: Cannot read property 'id' of undefined ; Zone: <root> ; Task: Promise.then ; Value: TypeError: Cannot read property 'id' of undefined at registerNgModuleType (core.umd.js:24829) at core.umd.js:24840 at Array.forEach (<anonymous>) at registerNgModuleType (core.umd.js:24840) at new NgModuleFactory (core.umd.js:24936) at compileNgModuleFactory__POST_R3__ (core.umd.js:28472) at PlatformRef.push../node_modules/@angular/core/bundles/core.umd.js.PlatformRef.bootstrapModule (core.umd.js:28716) at ElementRendererService.<anonymous> (ElementRendererService.js:89) at step (ElementRendererService.js:53) at Object.next (ElementRendererService.js:34) TypeError: Cannot read property 'id' of undefined at registerNgModuleType (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:76527:31) at http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:76538:51 at Array.forEach (<anonymous>) at registerNgModuleType (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:76538:21) at new NgModuleFactory (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:76634:17) at compileNgModuleFactory__POST_R3__ (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:80170:29) at PlatformRef.push../node_modules/@angular/core/bundles/core.umd.js.PlatformRef.bootstrapModule (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:80414:20) at ElementRendererService.<anonymous> (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:140803:26) at step (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:140767:23) at Object.next (http://localhost:6006/vendors~main.331aca382febe703f812.bundle.js:140748:53)

Does this mean storybook need a compiled ivy library to run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs angular feature request run e2e extended test suite Run the e2e extended test suite in CircleCI workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants