search

joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Experiment with Docz #254

Open joshwcomeau opened 6 years ago

joshwcomeau commented 6 years ago

Docz is an alternative to Storybook. It uses MDX, which is a cross between Markdown and JSX, and it's really nice for this kind of thing.

I gave it a quick try with our Button component, and I quite like it:

screen shot 2018-09-17 at 9 22 26 am

I uploaded it to Surge, play with it here: http://guppy-docz.surge.sh/src-components-button-stroke-button

Pros:

Cons:

Really curious what y'all think. I think I'm leaning towards Docz, but it's not a strong feeling. I also expect Docz might improve a lot over the next few months, so maybe we can revisit this later?

j-f1 commented 6 years ago

MDX doesn't yet support Prettier. They've already merged a PR for it, but it isn't released yet.

You can always install prettier/prettier to get the bleeding-edge versión from GitHub.

joshwcomeau commented 6 years ago

ok, I think it's worth switching to Docz. It's easier to write stories, and the end result is nicer / more usable, IMO.

Will add a new issue

joshwcomeau commented 6 years ago

Heads-up, if anyone's interested: I'm gonna convert all of our existing stories to docz in the next week or so. I have some good ideas for how this could become our style-guide, making it easier for folks to contribute to the UI :D

j-f1 commented 6 years ago

This branch is now properly deployed to Netlify. It looks like the homepage is not defined — should we put something there?

joshwcomeau commented 6 years ago

It looks like the homepage is not defined — should we put something there?

Yep, I'm on it.

I'm excited to treat Docz as more of a "style guide". It should help developers understand how to contribute to the UI, not just be a component library that shows what props are available (although it should do that too).

So the homepage will include some info about what it's for, and links to common UI pieces.

joshwcomeau commented 6 years ago

@j-f1 it looks like client-side nav links aren't respected (the link you provided is dead, since netlify doesn't know to redirect that URL to the homepage). Is there a way to set up Netlify to handle client-side navigation?

melanieseltzer commented 6 years ago

@joshwcomeau Is it handled by a router? Perhaps this will help.

j-f1 commented 6 years ago

All fixed @joshwcomeau! I used a similar technique to the one @melanieseltzer suggested, but I put it in the netlify.toml file instead.

AWolf81 commented 5 years ago

@joshwcomeau I've modified the structure of the docz a bit and added an index route. Nothing pushed yet - it's just locally. If you like I can push it to docz branch.

The content of index page Guppy Style Guide in my WIP is just to have some content and should be improved.

grafik

I really like the new docs from react-spring with docz. Maybe we can do it similar but I think we need more structure like in my screenshot above.

I'd also like to help to port the storybook stories to docz. What do you think can we merge the basic setup from here to master and create an new issue for tracking the work?

j-f1 commented 5 years ago

Prettier supports MDX now 🎉

superhawk610 commented 5 years ago

I think it's safe to say that docz and MDX are here to stay. This PR will need to be rebased and brought up to speed with master, then all remaining stories will need to be converted to docz. I'll handle the rebase tonight, then anyone who'd like to champion the rewrite feel free to chime in below!

EDIT: There have been some big changes to docz in the last few weeks, and it looks like a few things are required to move forward with this PR:

EDIT 2: Currently, there's no way to have 3rd+ layer nested menu items, so we'll go with a flatter structure for now and track progress here.

EDIT 3: The activeClassName and partiallyActive DOM errors on every page are being tracked here. The 1.0 release is only a few days old at this point so there's still a few bugs to be ironed out, it seems.

EDIT 4: Both Flow and styled-components currently require some extra config to work correctly with the <Props /> component, which is a core feature of docz. If this workflow isn't improved in the v1 release then docz may not in fact be a good fit for this app. There's also a soft dependency on styled-components@^4.0.0, which we're not currently using (see #213 to track progress). Let's check back in a couple weeks to determine if the future of Guppy+docz is promising or not.

codecov[bot] commented 5 years ago

Codecov Report

Merging #254 into master will decrease coverage by 0.74%. The diff coverage is 2.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   58.83%   58.08%   -0.75%     
==========================================
  Files         158      163       +5     
  Lines        3357     3400      +43     
  Branches      467      471       +4     
==========================================
  Hits         1975     1975              
- Misses       1179     1218      +39     
- Partials      203      207       +4
Impacted Files Coverage Δ
...CreateNewProjectWizard/Gatsby/SelectStarterList.js 100% <ø> (ø) :arrow_up:
src/components/Heading/Heading.js 75% <ø> (ø) :arrow_up:
src/docz/components/ProgressManager/index.js 0% <0%> (ø)
src/components/OnlineChecker/OnlineChecker.js 0% <0%> (ø) :arrow_up:
src/docz/components/Toggleable/index.js 0% <0%> (ø)
src/docz/components/Toggleable/Toggleable.js 0% <0%> (ø)
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ø) :arrow_up:
...docz/components/ProgressManager/ProgressManager.js 0% <0%> (ø)
src/docz/colors/ColorList.js 0% <0%> (ø)
src/utils.js 39.04% <100%> (ø) :arrow_up:
... and 5 more
melanieseltzer commented 5 years ago

@superhawk610 I've been using Docz for one of my side projects and it's been working well. It seems to be more stable now at 1.1.0, so I went ahead and updated this branch :)

Item 3 and 4 from your list seem resolved. Unsure about 2, haven't looked into it.

We are experiencing https://github.com/pedronauck/docz/issues/838, tracking that thread. Doesn't seem to be causing error, just throwing up a warning.

We experience https://github.com/pedronauck/docz/issues/793 but adding react-hot-loader as an explicit devDep to update it fixes it for now.

What do you think?

[Edit] Not sure why Flow is failing in CircleCI...

[Edit2] Huh CI is fixed now 🤷‍♀

superhawk610 commented 5 years ago

I'm OK with pinning peer dependencies so the react-hot-loader thing doesn't bother me (I wish package.json supported inline comments so we could note why we're pinning it but that's another discussion entirely).

As for 2, I'm personally a fan of nested menu structures, eg

Components
  Buttons
    OutlineButton
    FillButton
  Animated
    LoadingSpinner
    GuppyLoader
Styles
  General
  All

but we can definitely get by with a flat structure, so that's not really a blocker.

3 and 4 were the largest blocks IMO so now that they're fixed I think this should definitely become a priority PR. What do you think is left to do before this can be merged?

melanieseltzer commented 5 years ago

I don't think there's a crazy amount to do, I'll probably try tackle some of this list this week or weekend :)

Adding new mdx files can come in a new PR, we can focus on getting Docz to where Storybook is and then expand later.

Will the URL still be https://guppy.netlify.com? I see the netlify.toml in this branch with the updated build command, but will it only build once this is merged to master?

AWolf81 commented 5 years ago

@melanieseltzer Sorry, I can't find the stuff I created in November - seems like I didn't commit that. No local docz branch & nothing about it in reflog.

For deploying: It's possible to do a branch deploy & it seems that it is enabled as deploying task is started but it fails with a message like build-docz not found. See my inline comment on how to fix it - just the wrong name in netlify.toml.

melanieseltzer commented 5 years ago

@AWolf81 No worries, I'll use the stuff from your screenshot and build on that 👍 Thanks for spotting the wrong command! It's up now with a deploy preview.

melanieseltzer commented 5 years ago

@superhawk610 @AWolf81 Alright, lingering Storybook stories have been converted, and cleaned things up a bit (also added the logo!). Had to add Docz-specific components (src/docz/components) because those logics were in the storybook js files directly, and mdx can only import components.

Is there anything you think we could improve upon before a merge?

AWolf81 commented 5 years ago

@melanieseltzer Looks great. Well done. 👍

I'll do a review later today.

For the styling I have one small point - I think it would look better if the Guppy logo is centered in the sidebar. What do you think?

grafik

In the docz I've noticed that in ProjectIconSelection selecting an icon is not working. It would be great if the selection is working.

It would be also great if in the index page there would be a short explanation how to add new Docz stories so new devs are getting an idea where and how to add them. Just adding an small example .mdx and that the store will live in the same directory as component will be a good starting point. Also a link to docz getting started docs would be nice.

Any ideas why Circle CI is failing? I've tried to re-build but Flow is failing. I thinks it's a CircleCI issue and not with our code.

melanieseltzer commented 5 years ago

Agreed, centered logo looks better, updated it + expanded the getting started info per your suggestions 👍 Will punt on selecting an icon on click for ProjectIconSelection, was also thinking it could be good to show.

I'm also confused why Flow is failing in CircleCI, it looks like it's also happening in #374 as well. No problems locally so this is confusing...

superhawk610 commented 5 years ago

Unfortunately, I don't have any suggestions as to how to resolve the Flow CI issue, but I did experience it quite a bit when actively working on this project (especially on Windows). I could never reliably reproduce it but it cropped up every other day or so and nothing consistently solved it (nuking node_modules, using global/local flow-bin, upgrading/downgrading, etc). That's one of the major reasons I switched to TS exclusively, but obviously that's not an option for this project.

Here is a thread dating back almost 4 years showing that this issue continues to crop up. The common response is "mismatched global local versions" but I don't know how that could be the case in CI. Maybe we need to bump the flow-bin version?

AWolf81 commented 5 years ago

@superhawk610 I'm running out of ideas - I've tested some of the comments from this issue but with-out success.

I've tried:

On Windows it's also really slow for me and VS code liniting is disabled as two running servers are a problem for flow.

I think converting to Typescript is probably a pretty heavy task as we would have to modify many files - not sure if it's worth it. But would be an idea if we can't fix the flow issue.

[Edit]: I think Netlify is failing because I've updated the package.json in Github. I'll check yarn.lock file later - maybe there is a problem.

superhawk610 commented 5 years ago

@AWolf81 perhaps this is the solution?

That thread was opened because of the exact errors we're now getting on CircleCI.

AWolf81 commented 5 years ago

@superhawk610 thanks for pointing to that issue.

I think settings the max workers fixed it - not sure why. Next we need to check why Flow is failing - is it because of the previous flow check addition or is it a type error?

superhawk610 commented 5 years ago

@AWolf81 looks to be a type error, I'll see if I can patch it.

EDIT: Alright, that got it. What should we do about the coverage? I wouldn't imagine Docz stuff should really be considered in coverage.

codecov[bot] commented 5 years ago

Codecov Report

Merging #254 into master will decrease coverage by 0.89%. The diff coverage is 1.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #254     +/-   ##
=========================================
- Coverage   58.83%   57.93%   -0.9%     
=========================================
  Files         158      165      +7     
  Lines        3357     3409     +52     
  Branches      467      471      +4     
=========================================
  Hits         1975     1975             
- Misses       1179     1227     +48     
- Partials      203      207      +4
Impacted Files Coverage Δ
...CreateNewProjectWizard/Gatsby/SelectStarterList.js 100% <ø> (ø) :arrow_up:
src/components/Heading/Heading.js 75% <ø> (ø) :arrow_up:
src/docz/components/ProgressManager/index.js 0% <0%> (ø)
src/components/OnlineChecker/OnlineChecker.js 0% <0%> (ø) :arrow_up:
src/docz/components/Toggleable/index.js 0% <0%> (ø)
src/docz/components/Toggleable/Toggleable.js 0% <0%> (ø)
src/components/LoadingScreen/LoadingScreen.js 0% <0%> (ø) :arrow_up:
src/docz/components/IconSelector/IconSelector.js 0% <0%> (ø)
...docz/components/ProgressManager/ProgressManager.js 0% <0%> (ø)
src/docz/colors/ColorList.js 0% <0%> (ø)
... and 9 more
AWolf81 commented 5 years ago

@superhawk610 yes, we can exlclude it from coverage by updating this line in package.json.

What do you think should we revert the change with flow check? For me flow is not starting locally with it. But I'm not sure what's the difference between check/server & status. Even after reading this SO.

superhawk610 commented 5 years ago

I believe flow check is the correct command to use for CI since it's self-contained - it starts and stops discretely. I can confirm flow check works locally for me on Mac, but it's always worked better on Mac than Windows.

melanieseltzer commented 5 years ago

@AWolf81 I got the project icon selection working, let me know what you think?

Btw, flow check works for me locally on Mac but it took a while to finish checking.

AWolf81 commented 5 years ago

I wanted to check Docz but my build with yarn docz:dev is not working.

On first try it complains about missing @mdx-js/react. Once it's installed it fails with the following error in browser: grafik

That's also mentioned in this issue but the React dependency seems correct. I also tried to reinstall node_modules & recreating yarn-lock file.

But still not working. My setup:

I also tried Docz@next but that failed with a build error ./src/constants not found and I'm not sure how to fix it.

rakannimer commented 4 years ago

Hey all !

I'm the maintainer of docz and a big fan of guppy !

I'd love to help you get setup quickly with docz v2 if you're still interested.

I can open a PR to the docz branch that you could merge before this PR.

Sounds good ?

cc @joshwcomeau

superhawk610 commented 4 years ago

Hey @rakannimer, that sounds awesome! We first opened this branch right around the time docz was making the v1 -> v2 transition, so I figured we would wait until things had settled down and some of the growing pains had been worked out before revisiting.

Briefly checking the status of some of the aforementioned issues seems to indicate that most of them have been resolved, which is great! I remember one of the biggest issues we had is that docz didn't seem to recognize Flow types when using <Props of={Component} />, which is problematic since the codebase doesn't use TS and relies solely on Flow for typing 🙃.

Additionally, do you think we'll be able to use styled-components@3.x with the codebase and styled-components@4.x within docz? Or will this PR be dependent on getting sc@4.x implemented throughout the codebase?

rakannimer commented 4 years ago

Hey @superhawk610

Thanks for the quick reply ! I'll answer your concerns below as best as I can.

I figured we would wait until things had settled down and some of the growing pains had been worked out before revisiting.

Smart move, it took us quite a bit of time to move from 2.0 release candidates to a solid v2. We're still discovering and fixing bugs of course, but hopefully nothing too major or blocking and we setup e2e tests and CI which will help us keep moving fast with confidence !

one of the biggest issues we had is that docz didn't seem to recognize Flow types when using , which is problematic since the codebase doesn't use TS and relies solely on Flow for typing

docz should understand flow types just as well as typescript ones, I just tested out the latest version with the flow example and it worked as expected. If for some reason, it doesn't recognize the types in this repo than it's probably a bug that we will definitely fix 👍

Additionally, do you think we'll be able to use styled-components@3.x with the codebase and styled-components@4.x within docz

To the best of my knowledge docz v2 doesn't have a dependency on styled-components, so we should be all right using v3 but I will give this a try to confirm.

styled-components currently require some extra config to work correctly with the component

This might be true depending on how you're defining your components but it shouldn't be anything too major, I'll setup an example to demonstrate and report back.

This was fixed starting from 2.2.1-alpha.1 and the <Props /> component now works with styled-components without any additional config 👍

Example : https://github.com/doczjs/docz/tree/master/examples/styled-components