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

Restore options 31 #32

Closed
wants to merge 22 commits into from
Closed

Restore options 31 #32

wants to merge 22 commits into from

Conversation

jdungan
Copy link
Member

@jdungan jdungan commented Dec 29, 2014

This is a start to #31. it allows the user to save a string in local storage. Next step sending the token with bid requests.

});


gulp.task('minify', function () {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a minify task because

  1. The extension code will be zipped up and delivered via Chrome Web Store
  2. The code is all open-source, so no need to obfuscate/minify/uglify it

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm working on a 'publish' task to compile and pack the zip file. Minifying was to compress even more. Plus it's only the compiled js not the coffee src.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep the build/publish tasks as simple as possible: removing minify task lets us remove 30 lines of gulp configuration and 3 dependencies.

@groovecoder
Copy link
Member

When I ran gulp coffee I got errors about having to install all the dependencies at the top of gulpfile. Can we create a package.json with appropriate dependencies so we can just npm install to get everything at once?

@groovecoder
Copy link
Member

There's no options_ui manifest field where I can enter the auth token?

@groovecoder
Copy link
Member

We should also add these files to .gitignore:

.DS_Store
node_modules

I also filed an issue to remove .js files from the repo and .gitignore them.

@groovecoder
Copy link
Member

But despite all that ...
giphy

codesy-chrome-extension

@groovecoder
Copy link
Member

The only things I'd like to block the merge are:

  • Remove the minify task and the associated config and dependencies
  • Add a package.json to enable npm install
  • Add node_modules to .gitignore

The rest can be filed as issues or cleaned up in the course of other work.

@@ -1 +1,3 @@
*.swp
node_modules
pkg
Copy link
Member

Choose a reason for hiding this comment

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

need to add .DS_Store

@groovecoder
Copy link
Member

Superseded by #40

@groovecoder groovecoder closed this Jan 3, 2015
@groovecoder groovecoder deleted the restore-options-31 branch January 3, 2015 21:15
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.

None yet

2 participants