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

Chore: update angular documentation #228

Merged
merged 15 commits into from
Jan 24, 2020

Conversation

jonniebigodes
Copy link
Collaborator

@jonniebigodes jonniebigodes commented Dec 7, 2019

With this pr, the intro to storybook tutorial for angular is updated. It was long overdue, and for that i would like to apologize to @geromegrignon for putting this on the "back burner" (pardon the bad pun).

It will close #213 #104 #227 and #60 .

The updated code is in here

Some caveats:

  • I've added the do not merge label for now as there's still a lingering issue with angular and storybook as mentioned in here i'm going to test it out shortly, but for now i would like @geromegrignon 's input on this, as he expressed some knowledge in angular, probably more even than me.
  • In the deploy section, the settings image for netlify was updated, to avoid confusion, preventing the build from completing. as it's mentioning yarn and the tutorial is "operating" with npm and netlify will pick up on package-lock.json file and go from there.
  • Added a message in the translations available, so that when the reader picks up on it. It will know that they are not updated yet. Probably bring in more and more people to help out with the translations.
  • The code examples for stories are not using CSF. I think that could be addressed a bit later in a subsequent pr.

@jonniebigodes jonniebigodes added the DO NOT MERGE In progress PRs label Dec 7, 2019
A recent pr that was merged will mislead people with angular.
Generating errors in netlify that the dir does not exist.
@geromegrignon
Copy link
Contributor

I'll try this new version to review it next week.

jonniebigodes and others added 2 commits December 21, 2019 22:52
@jonniebigodes jonniebigodes removed the DO NOT MERGE In progress PRs label Dec 21, 2019
@jonniebigodes
Copy link
Collaborator Author

jonniebigodes commented Dec 21, 2019

With the latest commit the angular version of the tutorial was updated to csf to align itself with the remainder of the documentation. Also the issue that i was experiencing with jest+ storyshots addon is now fixed and the Do not merge label was removed.

Feel free to provide feedback

"outDir": "./out-tsc/spec",
"module": "commonjs",
"allowJs": true,
"types": ["jest", "jquery", "jsdom", "node"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider to be more specific about the changes as users might rely on the tutorial to integrate storybook to their project with already changes to this file.

"moduleResolution": "node",
"importHelpers": true,
"target": "es2015",
"typeRoots": ["node_modules/@types"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider to be more specific about the changes as users might rely on the tutorial to integrate storybook to their project with already changes to this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@geromegrignon probably a better wording for this could be:

In your tsconfig.spec.json add the following keys and values to the compilerOptions:

{
 "compilerOptions": {
  ....
  "module": "commonjs",
  "allowJs": true,
  }
}

You'll also need to change the types to the following:

{
 "compilerOptions": {
  "types": ["jest", "jquery", "jsdom", "node"]
 }
}

Moving onto tsconfig.json. Once again under compilerOptions add the following key and value emitDecoratorMetadata: true

And finally in tsconfig.app.json add a reference to the folder you've created earlier to the exclude, turning its contents into:

{
  "exclude": ["src/test.ts", "src/**/*.spec.ts", "**/*.stories.ts", "src/jest-config"]
}

Do you agree with this change?

```

<div class="aside">
<p>We’ve used <code>svn</code> (Subversion) to easily download a folder of files from GitHub. If you don’t have subversion installed or want to just do it manually, you can grab the folders directly <a href="https://github.com/chromaui/learnstorybook-code/tree/master/public">here</a>.</p></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would place this part before the commands to make it more accessible : explain then act.
And be more explicit about what users have to do once files are grabbed (ie copy them into the assets folder)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@geromegrignon that section needs a bit of work, i've been thinking about this not only for this section but for the remainder of the documentation. Probably a better wording for this would be (posting it as i have it on my end):

## Add assets

To match the intended design, you'll need to download both the font and icon directories and place its contents inside your `assets` folder.
<div class="aside">
<p>We’ve used <code>svn</code> (Subversion) to easily download a folder of files from GitHub. If you don’t have subversion installed or want to just do it manually, you can grab the folders directly <a href="https://github.com/chromaui/learnstorybook-code/tree/master/public">here</a>.</p></div>
svn export https://github.com/chromaui/learnstorybook-code/branches/master/public/icon assets/icon
svn export https://github.com/chromaui/learnstorybook-code/branches/master/public/font assets/font
After adding styling and assets, the app will render a bit strangely. That’s OK. We aren’t working on the app right now. We’re starting off with building our first component!

Do you agree with this? also @tmeasday and @domyen , do you both agree with the change?

- `title` -- how to refer to the component in the sidebar of the Storybook app,
- `excludeStories` -- information required by the story, but should not be rendered by the Storybook app.

To define our stories, we export a function for each of our test states to generate a story. The story is a function that returns a rendered element (i.e. a component class with a set of props) in a given state---exactly like a React [Stateless Functional Component](https://reactjs.org/docs/components-and-props.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the react link to the angular documentation : https://angular.io/guide/component-interaction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@geromegrignon i left it temporarily. I was doing some research on this and it would seems that a better wording could be "....exactly like a Stateless Functional Component, leaving out React out of the "picture". Do you agree?

content/intro-to-storybook/angular/en/simple-component.md Outdated Show resolved Hide resolved
@jonniebigodes
Copy link
Collaborator Author

@geromegrignon i've incorporated the changes in the latest commit, care to take a look at them and see if that's ok with you?

@jonniebigodes
Copy link
Collaborator Author

With the latest commit the documentation was updated to match Storybook version 5.3.
In order to align the documentation properly, all of the components now "live" inside the components folder to avoid having the angular documentation point to one thing and the rest to another place.

I've updated the repo and everything is working fine, asides from the issue that i've experienced with the svelte documentation that despite the packages being bumped to 5.3.5 it still throws the same error.

Feel free to provide feedback

@tmeasday
Copy link
Member

@jonniebigodes is this the same as the version I reviewed accidentally the other day? Then I am good with it.

@jonniebigodes
Copy link
Collaborator Author

@tmeasday yes..yes it is...the pr for the other piece of documentation was made from this branch, so this angular documentation "tagged along" with it. Also if you're ok, i'm ok with it aswell. Feel free to merge this and i'll address the translation shortly. If you don't mind i'll go framework by framework

@tmeasday tmeasday merged commit 6952a51 into master Jan 24, 2020
@tmeasday tmeasday deleted the chore/intro_to_storybook_angular_updates branch January 24, 2020 05:50
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.

Angular documentation includes a React screenshot
5 participants