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

webpack integration #272

Merged
merged 16 commits into from
Apr 4, 2018
Merged

webpack integration #272

merged 16 commits into from
Apr 4, 2018

Conversation

dreamkyr
Copy link
Contributor

@dreamkyr dreamkyr commented Mar 28, 2018

test function still needs more work, but others are working perfectly.


This change is Reviewable

@xarg
Copy link
Contributor

xarg commented Mar 28, 2018

The content script is breaking my gmail style (seems the body styles are not restricted to gorgias chrome popup anymore):

Selection_004.png

Please automatically format the files (number of spaces, etc..)


Reviewed 14 of 16 files at r1.
Review status: 14 of 16 files reviewed at latest revision, all discussions resolved.


Gruntfile.js, line 3 at r1 (raw file):

'use strict';

module.exports = function (grunt) {

gruntfile should be deleted no?


webpack.config.js, line 5 at r1 (raw file):

    stylus = require("stylus")
    fileSystem = require("fs"),
    jshint = require("jshint-loader")

missing commas here. Also at Gorgias we prefer one declaration per line (it's our convention). Example:

var a = 1;
var b = 2;
etc..


config/webpack.test.js, line 4 at r1 (raw file):

exports.generateCrx = ({}) => {
    

please use an editor that understands .editorconfig like WebStorm.


src/manifest.json, line 41 at r1 (raw file):

            ],
            "css": [
                "background/css/background.css"

that's a big no-no. the content.css is specific for the content script part of the extension.


src/background/css/installed.styl, line 1 at r1 (raw file):

@import "../../../node_modules/bootstrap/dist/css/bootstrap.css";

can you check if @import "~bootstrap/dist..." works?


src/pages/views/installed.html, line 1 at r1 (raw file):

<link href="../background/css/installed.css" rel="stylesheet" type="text/css" media="screen" charset="utf-8">

are you sure here?


Comments from Reviewable

@xarg
Copy link
Contributor

xarg commented Mar 28, 2018

Review status: 14 of 18 files reviewed at latest revision, 6 unresolved discussions.


README.md, line 25 at r2 (raw file):

* `yarn start` - Development mode. Creates development manifest, watches for styl files and recompiles them automatically.
* `yarn build` - Build extension and compress extension.
* `grunt test` or `grunt t` - Run tests.

npm test no?


Comments from Reviewable

@xarg
Copy link
Contributor

xarg commented Mar 28, 2018

remove bower_components from the repo


Reviewed 2 of 16 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.


Comments from Reviewable

@dreamkyr
Copy link
Contributor Author

currently all style files merged into one file: background.css.

@xarg
Copy link
Contributor

xarg commented Mar 28, 2018

Re: background.css - that's not an option since in the content side we have a separate css which is a lot smaller and specific for the content side.

@dreamkyr
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 7 unresolved discussions.


Gruntfile.js, line 3 at r1 (raw file):

Previously, xarg (Alex Plugaru) wrote…

gruntfile should be deleted no?

grunt will be removed completely after fixing test function with webpack.


Comments from Reviewable

@dreamkyr
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 7 unresolved discussions.


README.md, line 25 at r2 (raw file):

Previously, xarg (Alex Plugaru) wrote…

npm test no?

will write npm test after fixing test with webpack.


webpack.config.js, line 5 at r1 (raw file):

Previously, xarg (Alex Plugaru) wrote…

missing commas here. Also at Gorgias we prefer one declaration per line (it's our convention). Example:

var a = 1;
var b = 2;
etc..

got it


config/webpack.test.js, line 4 at r1 (raw file):

Previously, xarg (Alex Plugaru) wrote…

please use an editor that understands .editorconfig like WebStorm.

got it.


src/manifest.json, line 41 at r1 (raw file):

Previously, xarg (Alex Plugaru) wrote…

that's a big no-no. the content.css is specific for the content script part of the extension.

will try to separate them.


Comments from Reviewable

@xarg
Copy link
Contributor

xarg commented Apr 2, 2018

Reviewed 743 of 743 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@dreamkyr
Copy link
Contributor Author

dreamkyr commented Apr 4, 2018

Review status: 748 of 760 files reviewed at latest revision, 1 unresolved discussion.


src/pages/views/installed.html, line 1 at r1 (raw file):

Previously, xarg (Alex Plugaru) wrote…

are you sure here?

it's merged into background.css


Comments from Reviewable

@xarg xarg merged commit 387990c into master Apr 4, 2018
@xarg xarg deleted the webpack-integration branch April 4, 2018 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants