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

Add vendor directories, exclude them from linting #178

Merged
merged 2 commits into from
May 11, 2017

Conversation

arturkot
Copy link
Contributor

@arturkot arturkot commented Apr 12, 2017

Hi everyone,

the purpose of this PR is to provide a way to easily add third party dependencies which, for various reasons, can't be installed from NPM. It usually applies to jQuery plugins or paid scripts which can't be exposed to all users. Same can go for CSS files.

Please let me know your thoughts!

Should (and can) I add any tests for this update? I'd need some directions here. :)

Cheers,

Artur

Copy link
Member

@luboskmetko luboskmetko left a comment

Choose a reason for hiding this comment

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

I'm wondering if it wouldn't be more reliable and flexible to use .eslintignore and .stylelintignore files with node_modules and vendor folders added to them by default. Then we or developers can easily add additional files and folders if needed.

@jakub300
Copy link
Collaborator

How should we use scripts/styles from vendor directories?

@arturkot
Copy link
Contributor Author

@luboskmetko Pushed! It works fine Mac OS. Would be cool to test those ignore files in Windows and Linux as well.

@jakub300

How should we use scripts/styles from vendor directories?

By using browserify-shim I think as Browserify is installed by default. Kind of like regular NPM packages. That's the most popular way (and problematic at the same time!) to add scripts judging on the dev help channel. :)

@luboskmetko
Copy link
Member

hey @arturkot can you please use only vendor instead of src/scripts/vendor and src/styles/vendor? In WP projects there is an option to move the src folder to the theme folder and I don't think the path would work then. Thanks

Copy link
Collaborator

@jakub300 jakub300 left a comment

Choose a reason for hiding this comment

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

Hey @arturkot,
If JS vendor directory is supposed to be used with browserify-shim could you write a proper guide how to use browserify-shim? It doesn't have to be part of that PR but it should finally happen.

Regarding this PR please verify what happens when src directory is moved to different place (there is option when creating WP project to do that).

@luboskmetko
Copy link
Member

luboskmetko commented May 9, 2017

@jakub300 @arturkot I've tested this when src moved to a different place, it works when you just use vendor folder instead of the whole path.

Agree regarding browserify-shim, but there is already an issue for that here #173

@arturkot
Copy link
Contributor Author

arturkot commented May 9, 2017

@jakub300 I'll add creating a guide to my to-do list. Thanks for pointing that out!

@arturkot
Copy link
Contributor Author

@luboskmetko Done!

Just need to point out one thing I've noticed. When I choose to move src into the theme directory (during yo setup), gulp build fails. Seems like the src in theme was incomplete (only some styles were copied). Manually copy-pasting the src from root into the theme fixed the issue.

I guess it's resolved in another PR?

Here's my yo config:

{
  "generator-chisel": {
    "config": {
      "name": "Some Random WP project",
      "author": "Artur",
      "projectType": "wp-with-fe",
      "nameSlug": "some-random-wp-project",
      "nameCamel": "Somerandomwpproject",
      "features": {
        "has_jquery": true,
        "has_babel": true
      }
    }
  }
}

Screenshot from terminal:

zrzut ekranu 2017-05-11 09 55 37

Theme's src:

zrzut ekranu 2017-05-11 09 57 43

cc @jakub300

@arturkot
Copy link
Contributor Author

@luboskmetko @jakub300 Please disregard the above comment. Sorry! Michal reminded me to rebase the branch which, of course, resolved the problem.

@luboskmetko luboskmetko merged commit 70c7284 into xfiveco:master May 11, 2017
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