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

Study@3 #21

Merged
merged 11 commits into from Jun 20, 2017
Merged

Study@3 #21

merged 11 commits into from Jun 20, 2017

Conversation

jonathanong
Copy link
Contributor

@jonathanong jonathanong commented Jun 17, 2017

closes #20

primary changes:

  • removes default stores
  • tests on jsdom
  • adds default and winner support
  • fixes a className bug

@jonathanong jonathanong force-pushed the study-rewrite branch 2 times, most recently from 0a750dc to 4ee42b8 Compare June 19, 2017 18:50
src/study.js Outdated
const { root } = this;
if (!root) return;


Copy link
Contributor

Choose a reason for hiding this comment

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

remove line

src/study.js Outdated
if (!root) return;


const currentClassNames = root.className.split(/\s+/g)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use classList here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't support returning all classes :/

if (!Array.isArray(data)) { normalizedData = [data]; }
define(tests) {
let normalizedData = tests;
if (!Array.isArray(tests)) normalizedData = [tests];

normalizedData.forEach((test) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove parens

previousAssignments,
userAssignments,
persistedUserAssignments,
} = this;

this.providedTests.forEach((test) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove parens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

airbnb eslint -_- we need to just make eslint-config-dsc

src/study.js Outdated
delete this.persistedUserAssignments[testName];
this.removeClasses(testName);
} else {
this.userAssignments[testName] =
Copy link
Contributor

Choose a reason for hiding this comment

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

eh? where's the right hand side of this assignment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right below it ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

i would refactor this to be more readable. took a lot of my mental energy to grok what was going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahaha i can make it one line instead

src/study.js Outdated
delete this.persistedUserAssignments[testName];
this.removeClasses(testName);
} else {
this.userAssignments[testName] =
Copy link
Contributor

Choose a reason for hiding this comment

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

i would refactor this to be more readable. took a lot of my mental energy to grok what was going on here.

@jonathanong jonathanong changed the title Study rewrite Study@3 Jun 20, 2017
@@ -0,0 +1,14 @@


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove new line

@jonathanong jonathanong merged commit 07c3dd2 into master Jun 20, 2017
@jonathanong jonathanong deleted the study-rewrite branch June 20, 2017 23:16
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.

refactor
2 participants