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

CLI: Add Ember stories and components #12304

Closed
wants to merge 7 commits into from
Closed

CLI: Add Ember stories and components #12304

wants to merge 7 commits into from

Conversation

tooppaaa
Copy link
Contributor

@tooppaaa tooppaaa commented Aug 28, 2020

Issue: #12173

What I did

  • Add Ember components
  • Create Ember stories based on templates
  • Update CLI generator to allow raw copy

How to test

  • E2E
yarn build cli
yarn test:e2e-framework ember --use-local-sb-cli

I only did not manage to have components using the .css, I include them directly in *.stories.js with a comment
I have a feeling this is a fragile implementation as we rely on a folder structure with ember-cli, it may be acceptable.
@gossi I'll do whatever feedback you might have on this :)

@tooppaaa tooppaaa added maintenance User-facing maintenance tasks cli ember labels Aug 28, 2020
@tooppaaa tooppaaa added this to the 6.1 essentials milestone Aug 28, 2020
@gossi
Copy link
Contributor

gossi commented Aug 28, 2020

Just looked roughly at the code. The code you used is one of the oldest ones in ember nowadays and nobody would do that any more. Please search for "Ember Octane" or go through the super rentals tutorial, you wanna use glimmer components with regular javascript classes. Please don't prefix components with storybook- - just do it without, it's a <Header>, a <Button> and whatever else :)

Also there is this one PR to link here: chromaui/learnstorybook.com#306 which kinda does something similar to here.
See the code for it: https://github.com/jonniebigodes/intro-to-storybook-with-ember

I think this enough for you to move into the right direction. Sorry if this is not more and precise. Atm my time just allows you to point at resources.

@dbendaou
Copy link
Member

dbendaou commented Sep 10, 2020

If you need to, I will be happy to help you on this merge request, either by doing the requested changes or while pairing on it! Let me know :)

@tooppaaa
Copy link
Contributor Author

Hey !

Sorry, quite busy at work at the moment, if you can help that would be awesome !! Feel free to commit on this branch, I'm available on Discord if needed :)

I can't say when I'll be able to spend some time on this :(

@dbendaou
Copy link
Member

Ok I'll try to work on this by the end of next week :) I'll keep you posted

@dbendaou
Copy link
Member

FYI I've started to work on this I should be able to have a PR ready by Friday hopefully

@dbendaou
Copy link
Member

Hey @tooppaaa, I've the PR in draft, how can I test/see that I didn't break anything of thoses files has it's not related to the ember-cli example ? Only E2E tests ?

@tooppaaa
Copy link
Contributor Author

@dbendaou Hey ! Thanks for your PR.
You can run

  • yarn build cli
  • yarn test:e2e-framework ember --use-local-sb-cli

That will build your CLI and use it to create a sample project in a siblings folder to your storybook folder.
That will also launch cypress.

The idea is for you to initialize the project using this command, do your changes and test them using cypress with re-running the above command, it's smart enough to skip initialization.

Last bit is for you to check that the components are good looking :)

@stale stale bot added the inactive label Oct 12, 2020
@yannbf
Copy link
Member

yannbf commented Oct 12, 2020

Adding a comment here so stalebot stops bothering. Any updates on this?

@stale stale bot removed the inactive label Oct 12, 2020
@dbendaou
Copy link
Member

Yes I'll finish my MR to update this one in the following days I didn't have any time so far unfortunately

@tooppaaa
Copy link
Contributor Author

I know the feeling @dbendaou... Thanks for taking this one !

@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 25, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2021

Fails
🚫 PR is marked with "in progress" label.

Generated by 🚫 dangerJS against 4b7ec9f

@gabrielcsapo
Copy link
Contributor

Working on this today @dbendaou!

@gabrielcsapo
Copy link
Contributor

@dbendaou updated the example app and the shipped default code. How do I test the changes located in cli/src/frameworks? Running;

yarn build cli
yarn test:e2e-framework ember --use-local-sb-cli

fails with a typescript error.

"storybook": "yarn build && start-storybook -p 9009 -s ember-output",
"storybook-prebuild": "yarn build && shx cp -r public/* ember-output",
"storybook:dev": "yarn dev & start-storybook -p 9009 -s ember-output"
"storybook:dev": "echo 'run `yarn dev` in another terminal' && start-storybook -p 9009 -s ember-output"
Copy link
Member

Choose a reason for hiding this comment

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

Should be first ember to start then running start-storybook in another terminal WDYT? To avoid concurrency issues btw ember not ready and storybook consuming ember output

@dbendaou
Copy link
Member

dbendaou commented Apr 9, 2021

@gabrielcsapo I didn't achieve to run e2e neither with

yarn build cli
yarn test:e2e-framework ember --use-local-sb-cli

Maybe @tooppaaa could help on this

Copy link
Contributor

@gossi gossi left a comment

Choose a reason for hiding this comment

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

what's the difference between these folders:

  • lib/cli/src/frameworks/ember
  • examples/ember-cli

I only see the PR diff, haven't looked deeply into storybook code for this. Just wondering why there are two similar things in two different folders?

Comment on lines 22 to +49
/**
* The hex-formatted color code for the background.
* @argument backgroundColor
* @type {string}
* @default null
*/
backgroundColor: null,
backgroundColor = null;

/**
* The hex-formatted color code for the subtitle.
* @argument subTitleColor
* @type {string}
*/
subTitleColor: null,
subTitleColor = null;

/**
* The title of the banner.
* @argument title
* @type {string}
*/
title: null,
title = null;

/**
* The subtitle of the banner.
* @argument subtitle
* @type {string}
*/
subtitle: null,
});
subtitle = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is no longer valid code for glimmer components. In typescript you would put this into an WelcomeBannerArgs interface and then do export default class WelcomeBanner extends Component<WelcomeBannerArgs>.

},
"dependencies": {
"@glimmer/component": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"@glimmer/component": "^1.0.0",
"@glimmer/component": "^1.0.4",

as of today we are at 1.0.4

subtitle=subtitle
click=(action onClick)
}}
<WelcomeBanner
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add a "deprecated" text here? With a link to use controls over knobs?

@shilman
Copy link
Member

shilman commented Apr 10, 2021

@gossi

  • the CLI folder is the template that gets copied over every time you run sb init on a new project
  • the examples/ember-cli is an example project that we use for internal testing when developing storybook

@tooppaaa
Copy link
Contributor Author

tooppaaa commented Apr 10, 2021

@gabrielcsapo @dbendaou, the issue in the e2e is related to yarn2. I'll follow up with our expert: @gaetanmaisse

 Configuring Yarn 2
touch yarn.lock && yarn config set npmScopes --json '{ "storybook": { "npmRegistryServer": "http://localhost:6000/" } }' && yar
n config set unsafeHttpWhitelist --json '["localhost"]' && yarn config set pnpFallbackMode none && yarn config set enableGlobal
Cache true && yarn config set "packageExtensions.react-scripts@*.peerDependencies.react" "*" && yarn config set "packageExtensi
ons.react-scripts@*.dependencies.@pmmmwh/react-refresh-webpack-plugin" "*" && yarn config set nodeLinker node-modules
Unknown Syntax Error: Extraneous positional argument ("storybook:").

$ yarn config set [--json] [-H,--home] <name> <value>
� Configuring Yarn 2 failed
� an error occurred:
Error: command exited with code: 1

@shilman
Copy link
Member

shilman commented Apr 10, 2021

@tooppaaa I've fixed the lockfile:

  • merge in next
  • "accept their changes" for yarn.lock
  • run yarn

@gabrielcsapo
Copy link
Contributor

Will update this this weekend. Sorry for the lag, had work priorities.

@yannbf
Copy link
Member

yannbf commented Oct 18, 2021

Will update this this weekend. Sorry for the lag, had work priorities.

Hey @gabrielcsapo! I was wondering if you are still planning to continue with the PR?

@MichaelArestad
Copy link
Contributor

Thanks everyone. I am closing this as it has been stale for some time. If you would like to further pursue this, just reopen the PR.

@stof stof deleted the feature/emberCli branch May 25, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli ember maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants