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

(complib) Add dev server for component docs #555

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Dec 22, 2017

overview

This PR adds a live-reloading documentation + playground dev server to the components library. Uses react-styleguidist. To use it:

cd components
make install
make dev

To keep this PR a little smaller, I've only added documentation for Buttons and Icons, and intend to follow up with the rest in separate PRs.

Fixes #546

changelog

  • (Chore) Add react-styleguidist-based dev server for component docs
  • (Docs) Document Button, PrimaryButton, and Icon components

review requests

Try this out on your own machine! It's pretty cool. This PR also required refactoring of the Button components as discussed in #546, but I think they ended up in a better place, anyway. Just please make sure I didn't break the app or PD

@mcous mcous added components Affects the `components` project docs ready for review refactor labels Dec 22, 2017
@mcous mcous force-pushed the complib_styleguide branch 2 times, most recently from e9911b8 to 25d564e Compare December 22, 2017 23:57
@@ -1,4 +1,7 @@
[ignore]
# ignore .json files in node_modules because some modules have malformed JSON
# in test fixtures (https://github.com/facebook/flow/issues/2364)
.*/node_modules/.*\.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I noticed flow typechecking in PD failed in CI for some reason (which was super weird because I didn't touch PD) and realized:

Some Googling led me to facebook/flow#2364. Turns out, react-styleguidist has a dependency called findup that has some empty json files in its test fixtures (which are not npm ignored). So what happens is:

  • The flow server starts and tries to parse the JSON
    • These files were coincidentally ignored in app and complib, but not pd
    • Why is flow parsing node_modules in separate projects?
      • Saw this behavior in app and pd
      • Willing to bet it's because @opentrons/components is symlinked in
  • Flow chokes on empty (i.e. malformed) JSON

To work around this, I added an ignore rule for JSON in node_modules. We should keep an eye on this in case it messes with @opentrons/components which is either inside or outside node_modules depending how symlinks are being followed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, yup, pretty sure ignoring JSON under node_modules broke the Windows build. Removing "Ready for Review" until this is fixed

@codecov
Copy link

codecov bot commented Jan 4, 2018

Codecov Report

Merging #555 into v3a will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              v3a     #555   +/-   ##
=======================================
  Coverage   71.07%   71.07%           
=======================================
  Files         156      157    +1     
  Lines        5318     5318           
  Branches       89       89           
=======================================
  Hits         3780     3780           
  Misses       1508     1508           
  Partials       30       30
Impacted Files Coverage Δ
components/src/icons/Icon.js 100% <ø> (ø) ⬆️
components/src/buttons/PrimaryButton.js 100% <100%> (ø)
components/src/buttons/Button.js 100% <100%> (ø)

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 d85f924...208796e. Read the comment docs.

@btmorr
Copy link
Contributor

btmorr commented Jan 4, 2018

On Windows (cygwin) after make install, I run make dev and get this:

⇒  make dev
node_modules/.bin/cross-env NODE_ENV=development node_modules/.bin/styleguidist server
Error: Cannot find module 'extract-text-webpack-plugin'
Error: Cannot find module 'extract-text-webpack-plugin'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (C:\cygwin64\home\bmorr\opentrons\opentrons\app\webpack\rules.js:4:27)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
make: *** [Makefile:39: dev] Error 1

Not sure if it's my environment, but I did npm install extract-text-webpack-plugin and still got the same error.

@mcous mcous force-pushed the complib_styleguide branch from be78129 to 13ad2c3 Compare January 4, 2018 20:02
Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

works on Linux. Looks good! Any clues about why it fails on Windows?

Do you have any thoughts about deploying v3a/develop/master to S3 so design team can see it more easily without noodling around on the command line?

@mcous
Copy link
Contributor Author

mcous commented Jan 4, 2018

Do you have any thoughts about deploying v3a/develop/master to S3 so design team can see it more easily without noodling around on the command line?

@IanLondon I think that's the logical next step. I know Netlify has some great features for deploying static sites automatically based on branch names

@IanLondon
Copy link
Contributor

Weird, extract-text-webpack-plugin only appears in app/ not in components/

I am getting linting errors in make test / make lint, but they are all "no-undef" and "no-unused-vars" about types, not the weird 0:0 error Parsing error: Cannot set property '0' of undefined null that Travis has

@IanLondon IanLondon added this to the Prototype designer prework milestone Jan 5, 2018
@mcous mcous added blocked Ticket or PR is blocked by other work and removed ready for review labels Jan 5, 2018
@mcous
Copy link
Contributor Author

mcous commented Jan 5, 2018

Given the weird issues we've been seeing here, I'm going to call this PR blocked by #576 because it really seems like lockfiles are misbehaving (missing dependency installs, etc)

@mcous mcous removed this from the Prototype designer: NavBar, SideBar, Step create/delete milestone Jan 9, 2018
@mcous mcous mentioned this pull request Jan 9, 2018
@mcous mcous force-pushed the complib_styleguide branch from 13ad2c3 to 208796e Compare January 9, 2018 23:00
@mcous mcous added ready for review and removed blocked Ticket or PR is blocked by other work labels Jan 9, 2018
@mcous
Copy link
Contributor Author

mcous commented Jan 9, 2018

With the latest version that uses yarn, I am able to run the dev server on Windows without problems!

@mcous mcous merged commit 9c0f0e9 into v3a Jan 9, 2018
@mcous mcous deleted the complib_styleguide branch January 9, 2018 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
components Affects the `components` project docs refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants