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

Update todoStorage.js #1593

Closed
wants to merge 11 commits into from
Closed

Update todoStorage.js #1593

wants to merge 11 commits into from

Conversation

githubERIK
Copy link

I experimented with the code and Deployd backend (http://deployd.com/). It seemed to me that clearCompleted function worked better with my proposed code.

@samccone
Copy link
Member

samccone commented Feb 3, 2016

@githubERIK looks like a few style issues on CI would you mind fixing those up

@githubERIK githubERIK closed this Feb 5, 2016
@githubERIK githubERIK reopened this Feb 5, 2016
@githubERIK
Copy link
Author

Managed to fix those style errors.

@passy
Copy link
Member

passy commented Feb 12, 2016

@githubERIK Could you explain why you want to change the code like this?

Also, it seems like you have found a loophole in our style guide, because this still looks very foreign compared to other code in that file and elsewhere. :)

@githubERIK
Copy link
Author

With the master branch code I got "DELETE (my_localhost_URL_ path) 400 (Bad Request)"
and the function did not delete records from Delpoyd and did not update the view. I changed the code and it seemed that it started to work. However, I just now noticed it will delete only one record. Because of that, I now changed the code again and it seems to work well (It will delete all completed records).
All in all, there is a chance that the original code in master branch did not work for me beacuse I might have modified my todoStorage.js http-paths the wrong way.

@addyosmani
Copy link
Member

Travis still seems to be complaining here 🐒

@githubERIK
Copy link
Author

Indeed. Could somebody explain me, what Travis means by the following output?

...
/home/travis/build/tastejs/todomvc/tests/node_modules/drool/lib/index.js:27
  return (new webdriver.WebDriver.Logs(driver))
          ^
TypeError: webdriver.WebDriver.Logs is not a function
...

@agraebe agraebe mentioned this pull request Mar 19, 2016
@githubERIK githubERIK closed this Mar 27, 2016
@githubERIK githubERIK reopened this Mar 27, 2016
@githubERIK
Copy link
Author

Pull request #1606 fixed Travis webdriver.WebDriver.Logs error. The Travis CI build passed.

});
for (var i = 0; i < completeTodos.length; i++) {
store.delete(completeTodos[i]);
};
Copy link
Member

Choose a reason for hiding this comment

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

No need to have a semicolon here. Kinda surprises me that the linter doesn't check for this? :/

}, function error() {
angular.copy(originalTodos, store.todos);
});
for (var i = 0; i < completeTodos.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Just use forEach? :)

@githubERIK githubERIK closed this May 9, 2017
@githubERIK githubERIK reopened this May 9, 2017
@githubERIK githubERIK closed this May 9, 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

Successfully merging this pull request may close these issues.

None yet

5 participants