Add stylelint CLI and Add mapbox class name rule in stylelintrc #4584

ai-santos · 2017-04-12T21:17:10Z

Launch Checklist

added stylelint as a dev dependency
added npm script lint-css
added rule in stylelintrc to make sure all selectors begin with mapbox
tested npm script in CLI to make sure it works
amended npm warning by adding || true to the script
briefly describe the changes in this PR

# This is the 1st commit message: Stylelint works for application # This is the commit message mapbox#2: WIP modify lint-css script in package.json # This is the commit message mapbox#3: Ammend warning for lint-css npm script

WIP modify lint-css script in package.json Ammend warning for lint-css npm script Add csscolorparser

ai-santos · 2017-04-13T01:35:28Z

@anandthakker - the Circle CI tests are failing but not for my PR issue.

mollymerp

Thank you for contributing to Mapbox GL JS @asantos3026!

The CI tests appear to be failing due to a missing flow type within the stylelint module. We do want to make sure that all css classes in dist/mapbox-gl.css are prefixed with mapboxgl-, am hesitant to add three new dependencies to solve this gap in coverage.

mollymerp · 2017-04-13T21:58:02Z

debug/access_token.js

@@ -1,6 +1,7 @@
 'use strict';

 mapboxgl.accessToken = getAccessToken();
+require('dotenv').config()


what was the reason for adding this change?

I was trying to access the mapbox API via a .env file that would include my keys. I realize I do not need to do that because I use the command line to get access to the API. I deleted line 4 above.

mollymerp · 2017-04-13T21:59:21Z

.stylelintrc

+{
+  "rules": {
+    "selector-class-pattern": "mapbox[a-z-]+",
+    "color-hex-case": "lower",


I'm not sure we need the color rules. we rarely change dist/mapbox-gl.css and we don't deal with much color in there.

Took out the color rules

mollymerp · 2017-04-13T22:01:46Z

package.json

@@ -18,6 +18,7 @@
    "@mapbox/whoots-js": "^3.0.0",
    "brfs": "^1.4.0",
    "bubleify": "^0.7.0",
+    "dotenv": "^4.0.0",


I dont think we need this dependency

took out the dotenv dependency

mollymerp · 2017-04-13T22:02:09Z

package.json

@@ -45,6 +46,7 @@
    "clipboard": "^1.5.12",
    "concat-stream": "^1.6.0",
    "coveralls": "^2.11.8",
+    "csscolorparser": "^1.0.3",


I think we can do without this dependency too

took out this dependency

mollymerp · 2017-04-13T22:02:13Z

package.json

@@ -115,8 +118,9 @@
    "start-docs": "npm run build-min && npm run build-docs && jekyll serve --watch",
    "lint": "eslint --cache --ignore-path .gitignore src test bench docs/_posts/examples/*.html debug/*.html",
    "lint-docs": "documentation lint src/index.js",
+    "lint-css": "stylelint 'docs/*.css' || true",


I think you should be pointing the linter at dist/mapbox-gl.css and not the docs/ css files. We only enforce the mapboxgl- prefex for css classes that ship with the library distribution. The CSS in the docs/ folder is only for the mapbox documentation website - www.mapbox.com/mapbox-gl-js/api
That change might also remove the need for the || true statement, although I'm not 100% clear on what that is doing in this case.

mollymerp · 2017-04-13T22:03:01Z

src/.env

@@ -0,0 +1 @@
+MAPBOX_ACCESS_TOKEN=sk.eyJ1IjoiYXNhbnRvczMwMjYiLCJhIjoiY2oxY293bGk4MDA2NDMzcGRxNGdyc3lpbSJ9.uxcZ_o19abopWFm2vwstmg


We'd like to avoid committing personal access tokens to the GitHub repo. Developers will need to use their own token with the library.

mollymerp · 2017-04-13T22:12:29Z

Ah just saw your discussion w @anandthakker on #4584 👍 on adding stylelint but would like to avoid the other two dependencies in this PR. Thanks again!

closes #4584

mollymerp · 2017-04-13T23:16:58Z

In order to get CI to pass based on I think you'll need to add a line ignoring stylelint in the .flowconfig file based on stylelint/stylelint#2322 (comment)

mollymerp · 2017-04-14T01:41:52Z

package.json

@@ -115,14 +116,15 @@
    "start-docs": "npm run build-min && npm run build-docs && jekyll serve --watch",
    "lint": "eslint --cache --ignore-path .gitignore src test bench docs/_posts/examples/*.html debug/*.html",
    "lint-docs": "documentation lint src/index.js",
+    "lint-css": "stylelint 'dist/mapbox-gl.css' || true",


can we remove the || true statements from here and the test-flow command – I'm afraid they will prevent the tests from failing if there are errors.

ai-santos · 2017-04-25T05:45:26Z

@mollymerp Hey Molly! I made all of the following changes:

removed true statement from test-flow and stylelint scripts
added stylelint to be ignored in the .flowconfig file

Let me know if there is anything else I need to do to clean up my PR 👍

mollymerp · 2017-04-25T17:06:04Z

great! thank you so much @asantos3026!!

ai-santos added 3 commits April 11, 2017 16:25

Add stylelint CLI and Add mapbox class name rule in stylelintrc

534f854

# This is a combination of 3 commits.

4f3b0ea

# This is the 1st commit message: Stylelint works for application # This is the commit message mapbox#2: WIP modify lint-css script in package.json # This is the commit message mapbox#3: Ammend warning for lint-css npm script

Stylelint implemented for application

ee56ee5

WIP modify lint-css script in package.json Ammend warning for lint-css npm script Add csscolorparser

ai-santos force-pushed the css-prefixes-stylelint branch from f70a41a to ee56ee5 Compare April 13, 2017 00:36

Add lintx to script file

2369f2c

ai-santos added 2 commits April 12, 2017 18:36

Took out lintx

60dca2d

Took out lintx

750b3f1

mollymerp suggested changes Apr 13, 2017

View reviewed changes

Resolve comments to 4584 PR

f64e2d2

Resolve PR comments

2078ec8

mollymerp reviewed Apr 14, 2017

View reviewed changes

Ignore stylelint in .flowconfig and remove true from scripts

a2582c0

mollymerp approved these changes Apr 25, 2017

View reviewed changes

mollymerp merged commit 5ceec40 into mapbox:master Apr 25, 2017

This was referenced Apr 25, 2017

Use stylelint to enfore css prefixes #4381

Closed

Prefix css #4642

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stylelint CLI and Add mapbox class name rule in stylelintrc #4584

Add stylelint CLI and Add mapbox class name rule in stylelintrc #4584

ai-santos commented Apr 12, 2017

ai-santos commented Apr 13, 2017

mollymerp left a comment

mollymerp Apr 13, 2017

ai-santos Apr 13, 2017 •

edited

Loading

mollymerp Apr 13, 2017

ai-santos Apr 13, 2017

mollymerp Apr 13, 2017

ai-santos Apr 13, 2017

mollymerp Apr 13, 2017

ai-santos Apr 13, 2017

mollymerp Apr 13, 2017

mollymerp Apr 13, 2017

mollymerp commented Apr 13, 2017

mollymerp commented Apr 13, 2017

mollymerp Apr 14, 2017

ai-santos commented Apr 25, 2017

mollymerp commented Apr 25, 2017

		@@ -0,0 +1 @@
		MAPBOX_ACCESS_TOKEN=sk.eyJ1IjoiYXNhbnRvczMwMjYiLCJhIjoiY2oxY293bGk4MDA2NDMzcGRxNGdyc3lpbSJ9.uxcZ_o19abopWFm2vwstmg

Add stylelint CLI and Add mapbox class name rule in stylelintrc #4584

Add stylelint CLI and Add mapbox class name rule in stylelintrc #4584

Conversation

ai-santos commented Apr 12, 2017

Launch Checklist

ai-santos commented Apr 13, 2017

mollymerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ai-santos Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Apr 13, 2017

mollymerp commented Apr 13, 2017

Choose a reason for hiding this comment

ai-santos commented Apr 25, 2017

mollymerp commented Apr 25, 2017

ai-santos Apr 13, 2017 •

edited

Loading