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

Documentation Site Skeleton #17

Merged
merged 18 commits into from
Mar 16, 2018
Merged

Documentation Site Skeleton #17

merged 18 commits into from
Mar 16, 2018

Conversation

Oreoz
Copy link
Collaborator

@Oreoz Oreoz commented Mar 14, 2018

Initial implementation of a documentation skeleton using ember-cli-addon-docs.

Linked to #15.

package.json Outdated
@@ -17,9 +17,16 @@
"license": "MIT",
"devDependencies": {
"broccoli-asset-rev": "^2.4.5",
"ember-ajax": "^3.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Which thing depends on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ember-cli-addon-docs depends on it currently, but it's not currently listed as a dependency.

ember-learn/ember-cli-addon-docs#76

@ef4
Copy link
Contributor

ef4 commented Mar 14, 2018

Thanks for getting this started, looks nice.

At least some of the test failures are legitimately detecting demos that don't look right anymore. We'll need to figure out why.

Oreoz and others added 4 commits March 14, 2018 14:44
- Reinstates the TimeLord to the `demos` route;
- Removes the uselss `demos/docs` template;
/* eslint-env node */
'use strict';

const AddonDocsConfig = require('ember-cli-addon-docs/lib/config');
Copy link
Contributor

Choose a reason for hiding this comment

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

Need

// eslint-disable-next-line node/no-unpublished-require

above line 4 to get tests to pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Just added this -- thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a closer look at this rule, and disabling it here is just a workaround.

The more complete solution is to create a files entry in our package.json. This would make it clear that config/addon-docs.js is not something we publish to consumers of the library, which would cause the unpublished rule to not worry about our use of a devDependency here.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Or instead of files we could also use .npmignore, which looks like it may be easier for this use case. Basically we just need to prove that config/addon-docs.js does not get published to npm.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see there is already an open issue for this on ember-cli-addon-docs: ember-learn/ember-cli-addon-docs#103

Copy link
Contributor

Choose a reason for hiding this comment

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

@Oreoz
Copy link
Collaborator Author

Oreoz commented Mar 15, 2018

@ef4 We got green tests -- thanks to @knownasilya. 🎉 I started converting your initial comments in sprite.js to JSDoc as a proof that API References will be automatically generated/maintained using this format.

@@ -14,7 +14,7 @@ export default DS.JSONAPIAdapter.extend({
id: String(i),
attributes: Object.seal({
name: faker.name.firstName(),
'avatar-url': faker.internet.avatar()
avatarUrl: faker.internet.avatar()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very confused why this was needed. Is ember-cli-addon-docs altering something about our default adapters?

The data returned here is supposed to look like a network request (meaning kebab-case is the standard). And it works on master the way I had it, but on this branch it only works with camelCase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it, ember-cli-addon-docs is indeed messing with the application serializer.

Copy link
Contributor

Choose a reason for hiding this comment

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

@knownasilya
Copy link
Contributor

Looks like tests are timing out.

@ef4
Copy link
Contributor

ef4 commented Mar 16, 2018

There are still demo problems that weren't caught by the test suite. I'm investigating.

For example, if you try running /demos/bind you'll see it doesn't move smoothly anymore.

And /demos/orphans animates in but not out.

@ef4
Copy link
Contributor

ef4 commented Mar 16, 2018

Disregard the part about /demos/orphans, I just realized it's also broken on master.

ef4 added 3 commits March 15, 2018 19:07
Because they're passing now and I want to know if that changes.
and drop unused bower.json because it causes some spurious errors in ember-try
@knownasilya
Copy link
Contributor

Some of the tests timeout and others error with Cannot find module 'recast'

@ef4
Copy link
Contributor

ef4 commented Mar 16, 2018 via email

@knownasilya
Copy link
Contributor

@ef4 are you using yarn to install locally? The ci is not, it says add useYarn: true to the config in ember-try I think.

@ef4
Copy link
Contributor

ef4 commented Mar 16, 2018

Yes, I'm running ember try:one directly just like in CI, so I'm also using npm. But that gives me an idea: I think I'm using a different npm version than travis.

To avoid possible npm5 issues
@knownasilya
Copy link
Contributor

Looking good so far, one passed 👍

@ef4 ef4 merged commit bd85e1a into ember-animation:master Mar 16, 2018
@knownasilya
Copy link
Contributor

Woot!

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.

3 participants