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

Roadmap #64

Open
7 tasks
beck opened this issue May 4, 2017 · 6 comments
Open
7 tasks

Roadmap #64

beck opened this issue May 4, 2017 · 6 comments

Comments

@beck
Copy link
Collaborator

beck commented May 4, 2017

This repo now has few more maintainers (including myself).

Before making any big changes would like to:

Say Hi

hi

Make a todo list

  1. Rewrite tests and use real browsers to run them: Rewrite tests  #69
  2. Fix ReferenceError: self is not defined #48
  3. Publish a new version
  4. Close my fork Deprecate this repo yola/classlist-polyfill#9
  5. Cleanup whitespace
  6. Minify via npm prepublish
  7. Start addressing open issues

Flesh out any preferences, paint a bikeshed

When writing JS I generally stick to airbnb-ish style.

For testing, prefer Mocha+chai (with karma running the tests) instead of QUnit.

Prefer plain old shell or JS scripts instead task managers.

@beck beck mentioned this issue May 4, 2017
@stevenvachon
Copy link

stevenvachon commented May 4, 2017

I fixed #48 in the "Consolidated closures" commit in #53.
I fixed whitespace in the "Whitespace fixes" commit in #53.
I rewrote the tests in the "Improve tests" commit in #53.

There are "..." buttons on those commits for you to read further commenting, in case you aren't familiar with GitHub.

prepublish is also ran on postinstall in npm <5. Likely not ideal at the moment. Use https://npmjs.com/in-publish

@beck
Copy link
Collaborator Author

beck commented May 4, 2017

@stevenvachon another preference is for single-purpose pull requests.

I agree with @eligrey about #53. IMO it does too much at once. Before the closures can be consolidated, lets cleanup the whitespace. Before we touch every line with whitespace cleanup, lets get the tests in working condition. I would expect a fix for #48 to have a diff of ~2 lines, something like:

-}(self));
+}(typeof self !== 'undefined' ? self : this));

@stevenvachon
Copy link

stevenvachon commented May 4, 2017

The whitespace commit occurs before the closure consolidation commit. You can click each commit to see its specific code changes.

@englishextra
Copy link

-}(self));
+}(typeof self !== 'undefined' ? self : this));

This is what I do with every polyfill I deal with.

BTW I'm gonna send a PR regarding !a===!b at the bottom of the polyfill, tha last thing which is a thorn.

@euwest
Copy link

euwest commented May 4, 2017

@stevenvachon:

another preference is for single-purpose pull requests.

I agree ... about #53. IMO it does too much at once.

That's @beck responding to:

I fixed #48 in the "Consolidated closures" commit in #53.
I fixed whitespace in the "Whitespace fixes" commit in #53.
I rewrote the tests in the "Improve tests" commit in #53.

He's telling you he would like those to be separate PRs, not just separate commits. Condescension is not an appropriate response.

@stevenvachon
Copy link

He's telling you he would like those to be separate PRs, not just separate commits. Condescension is not an appropriate response.

And I'm telling him and anyone else that they can go through the commits. I have little interest in redoing my PRs after all this time.

@beck beck mentioned this issue Jul 30, 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

No branches or pull requests

4 participants