(3rd review round) riot integration (good luck) #4070

libetl · 2018-08-24T16:05:05Z

Issue:

What I did

Egencia (my company) decided a while ago to use riot.js as its only web component framework.
Since no support is possible yet on storybook for that framework,
I tried to write a POC to be able to use storybook with the riot components

How to test

Is this testable with Jest or Chromatic screenshots?

I have written the kitchen-sink project in riot.js, and some jest tests

Does this need a new example in the kitchen sink apps?

Done. (alpha version)

Does this need an update to the documentation?

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

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

ndelangen

Super cool!, Left some comments, let me know if you need help?

ndelangen · 2018-08-24T18:40:00Z

app/riot/.npmignore

@@ -0,0 +1,3 @@
+docs


this should not be necessary

ndelangen · 2018-08-24T18:40:33Z

app/riot/.babelrc

@@ -0,0 +1,24 @@
+{


Are these configs very different from the one in the root?

Can we use the root's .babelrc ?

ndelangen · 2018-08-24T18:44:10Z

examples/riot-kitchen-sink/package.json

+    "@storybook/addon-storysource": "4.0.0-alpha.16",
+    "@storybook/addon-viewport": "4.0.0-alpha.16",
+    "@storybook/addons": "4.0.0-alpha.16",
+    "@storybook/riot": "../../app/riot",


should be a version as well, as the above packages, yarn workspaces makes this work.

ndelangen · 2018-08-24T18:46:05Z

examples/riot-kitchen-sink/src/index.js

@@ -0,0 +1,6 @@
+import riot from 'riot';
+// eslint-disable-next-line


Why disable eslint? if you're going to disable something, only disable specific rules.

ndelangen · 2018-08-24T18:51:36Z

examples/riot-kitchen-sink/src/stories/addon-backgrounds.stories.js

@@ -0,0 +1,27 @@
+import { storiesOf } from '@storybook/riot';
+import { withBackgrounds } from '@storybook/addon-backgrounds';
+import ButtonRaw from 'raw-loader!./Button.tag';


If loading .tag as raw, we should add it to the default webpack config for riot

The tag can be loaded as a component or as a string,
depends on developer's experience.
Some of them will instantiate the component with "mount" (with the riot-tag-loader)
And some of them will prefer to instantiate using a jsx-like notation (therefore with the raw-loader).

I meant to request changes

codecov · 2018-08-26T10:35:45Z

Codecov Report

Merging #4070 into master will decrease coverage by 0.03%.
The diff coverage is 36.69%.

@@            Coverage Diff             @@
##           master    #4070      +/-   ##
==========================================
- Coverage   40.58%   40.55%   -0.04%     
==========================================
  Files         469      483      +14     
  Lines        5642     5750     +108     
  Branches      749      771      +22     
==========================================
+ Hits         2290     2332      +42     
- Misses       2984     3045      +61     
- Partials      368      373       +5

Impacted Files	Coverage Δ
lib/cli/bin/index.js	`0% <ø> (ø)`	⬆️
lib/cli/bin/generate.js	`0% <0%> (ø)`	⬆️
...hots/storyshots-core/src/frameworks/riot/loader.js	`0% <0%> (ø)`
app/riot/src/server/options.js	`0% <0%> (ø)`
app/riot/src/client/preview/compileNow.js	`0% <0%> (ø)`
app/riot/src/server/wrapInitialConfig.js	`0% <0%> (ø)`
app/riot/bin/index.js	`0% <0%> (ø)`
app/riot/src/server/build.js	`0% <0%> (ø)`
app/riot/src/client/preview/globals.js	`0% <0%> (ø)`
app/riot/bin/build.js	`0% <0%> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb2396b...eda3f40. Read the comment docs.

igor-dv

Few more questions =)

docs/src/pages/basics/guide-riot/index.md

igor-dv · 2018-08-30T10:18:55Z

examples/riot-kitchen-sink/src/stories/addon-actions.stories.js

+import { action } from '@storybook/addon-actions';
+import ButtonRaw from './Button.txt';
+
+compileNow(tag, ButtonRaw);


Can compileNow get something that is not a tag?

sorry that is a bug will fix that right away

thanks for pointing that out.

lib/cli/generators/RIOT/template/stories/index.stories.js

igor-dv · 2018-08-30T10:28:49Z

lib/cli/lib/helpers.js

@@ -17,6 +17,9 @@ export async function getVersion(npmOptions, packageName, constraint) {
  } else if (/storybook/.test(packageName)) {
    current = devDependencies[packageName];
  }
+  if (packageName === '@storybook/riot') {


Why is this needed ?

oops, should have deleted that. Was there to make the build pass

igor-dv · 2018-08-30T10:29:33Z

lib/cli/test/fixtures/riot/package.json

+    "babel-register": "latest"
+  },
+  "peerDependencies": {
+    "babel-core": "^7.0.0 || ^8.0.0 || ^8.0.0-beta.6"


babel-loader

package.json

igor-dv · 2018-08-30T13:00:13Z

lib/cli/test/fixtures/riot/yarn.lock

@@ -0,0 +1,4675 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


I think yarn.lock is not needed in cli tests.

igor-dv · 2018-08-30T13:00:54Z

package.json

@@ -187,5 +192,8 @@
      "dependencies": "Dependency Upgrades",
      "other": "Other"
    }
+  },
+  "dependencies": {
+    "yarn": "^1.9.4"


igor-dv · 2018-08-30T13:10:23Z

I would like also understand the problem you have with tests. I think we should solve this and remove all the riot things from context.

igor-dv · 2018-08-30T13:17:22Z

I don't mind =)

libetl · 2018-08-30T15:09:05Z

@igor-dv : render-riot is now fixed.

@Hypnosphi @ndelangen @shilman @igor-dv don't hesitate to review when you get the chance.

libetl · 2018-08-31T05:06:22Z

🔥 🚀 let's start and maintain this project together. Thanks for your good reactivity.

igor-dv · 2018-08-31T07:17:36Z

Join our Slak

Hypnosphi · 2018-09-01T17:28:20Z

Released as 4.0.0-alpha.20

libetl requested review from Hypnosphi, igor-dv and ndelangen as code owners August 24, 2018 16:05

libetl changed the title ~~riot integration~~ [NOT READY, NOT ABLE TO JEST] riot integration Aug 24, 2018

libetl force-pushed the task/riot-integration branch from a5fcd8f to f7821e2 Compare August 24, 2018 16:07

ndelangen previously approved these changes Aug 24, 2018

View reviewed changes

igor-dv added the feature request label Aug 24, 2018

libetl force-pushed the task/riot-integration branch 8 times, most recently from 8631568 to cc21438 Compare August 25, 2018 22:25

libetl changed the title ~~[NOT READY, NOT ABLE TO JEST] riot integration~~ [NOT READY, JEST'ing] riot integration Aug 25, 2018

libetl force-pushed the task/riot-integration branch 3 times, most recently from 30a05ba to 6ceb89c Compare August 26, 2018 10:35

libetl force-pushed the task/riot-integration branch 6 times, most recently from 0cc3a41 to d561c64 Compare August 26, 2018 15:59

libetl changed the title ~~[NOT READY, JEST'ing] riot integration~~ (ready for review) riot integration Aug 26, 2018

libetl force-pushed the task/riot-integration branch from d561c64 to 42c6070 Compare August 26, 2018 16:32

libetl requested a review from shilman as a code owner August 26, 2018 16:32

igor-dv reviewed Aug 30, 2018

View reviewed changes

libetl added 4 commits August 30, 2018 12:32

changing the doc

d2dccac

compileNow should not need tag as param

7b8d35c

fixes

4b851f3

fixes

dce0411

libetl changed the title ~~(to review, again) riot integration (good luck)~~ (3rd review round) riot integration (good luck) Aug 30, 2018

igor-dv reviewed Aug 30, 2018

View reviewed changes

libetl added 2 commits August 30, 2018 15:09

removed yarn from package.json

68a8300

removing unneeded yarn.lock

7eb8044

libetl and others added 3 commits August 30, 2018 17:16

simplifying the render method (no more context object needed)

f555ce0

changing CONTRIBUTING.md

6a56f86

update of yarn.lock

eda3f40

Hypnosphi approved these changes Aug 30, 2018

View reviewed changes

Hypnosphi merged commit 5676863 into storybookjs:master Aug 30, 2018

igor-dv added the app: riot label Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(3rd review round) riot integration (good luck) #4070

(3rd review round) riot integration (good luck) #4070

libetl commented Aug 24, 2018 •

edited

ndelangen left a comment

ndelangen Aug 24, 2018

ndelangen Aug 24, 2018

ndelangen Aug 24, 2018

ndelangen Aug 24, 2018

ndelangen Aug 24, 2018

libetl Aug 24, 2018

codecov bot commented Aug 26, 2018 •

edited

igor-dv left a comment

igor-dv Aug 30, 2018

libetl Aug 30, 2018

libetl Aug 30, 2018

igor-dv Aug 30, 2018

libetl Aug 30, 2018

igor-dv Aug 30, 2018

igor-dv Aug 30, 2018

igor-dv Aug 30, 2018

libetl Aug 30, 2018

libetl Aug 30, 2018

igor-dv commented Aug 30, 2018 •

edited

igor-dv commented Aug 30, 2018

libetl commented Aug 30, 2018 •

edited

libetl commented Aug 31, 2018 •

edited

igor-dv commented Aug 31, 2018

Hypnosphi commented Sep 1, 2018

		@@ -0,0 +1,6 @@
		import riot from 'riot';
		// eslint-disable-next-line

		@@ -0,0 +1,4675 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

(3rd review round) riot integration (good luck) #4070

(3rd review round) riot integration (good luck) #4070

Conversation

libetl commented Aug 24, 2018 • edited

What I did

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?

ndelangen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2018 • edited

Codecov Report

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-dv commented Aug 30, 2018 • edited

igor-dv commented Aug 30, 2018

libetl commented Aug 30, 2018 • edited

libetl commented Aug 31, 2018 • edited

igor-dv commented Aug 31, 2018

Hypnosphi commented Sep 1, 2018

libetl commented Aug 24, 2018 •

edited

codecov bot commented Aug 26, 2018 •

edited

igor-dv commented Aug 30, 2018 •

edited

libetl commented Aug 30, 2018 •

edited

libetl commented Aug 31, 2018 •

edited