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

[docs] Demo codesandbox deps #10158

Merged
merged 5 commits into from
Feb 4, 2018
Merged

Conversation

caub
Copy link
Contributor

@caub caub commented Feb 3, 2018

I think a test will be safer, I've tried a few ones, it seems to work

Follow up for #10150

@caub caub force-pushed the demo-dyn-deps branch 2 times, most recently from a6eafee to a56aa47 Compare February 3, 2018 21:35
@oliviertassinari oliviertassinari changed the title [docs] Demo codesandbox deps, follow up for #10150 [docs] Demo codesandbox deps Feb 3, 2018
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Yeah! I was too lazy to push this forward. It's less code and more future proof. Good call 👍

@@ -55,6 +55,10 @@ module.exports = {
'import/unambiguous': 'off', // scripts
'import/namespace': ['error', { allowComputed: true }],
'import/no-extraneous-dependencies': 'off',
'import/order': ['error', {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@caub caub Feb 3, 2018

Choose a reason for hiding this comment

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

the nested array makes it so all groups can be mingled (other ref)

without the groups option, it defaults to their default group order, which makes a bunch of errors. I used it more for newlines-between to make sure import are together at the top of the file, easier to process

@@ -31,3 +31,19 @@ export function pageToTitle(page) {

return titleize(name);
}

export function getDependencies(raw) {
Copy link
Member

Choose a reason for hiding this comment

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

Some unit tests for this method would be perfect.

Copy link
Contributor Author

@caub caub Feb 3, 2018

Choose a reason for hiding this comment

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

done

Copy link
Member

@oliviertassinari oliviertassinari Feb 3, 2018

Choose a reason for hiding this comment

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

there are no tests for docs, should I put a helpers.spec.js next to it, (and add a npm script for testing also docs/**?

I think that adding the docs folder for the "test:unit" command is a good tradeoff. The tests won't be run in the CI, nor it will impact codecoverage. One simple test like the repl you linked should be enough :).

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Feb 3, 2018
@oliviertassinari oliviertassinari merged commit 2239f6f into mui:v1-beta Feb 4, 2018
@oliviertassinari
Copy link
Member

@caub good job, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants