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.

Sign up for GitHub

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

Jump to bottom

Add Rivets implementation + CR changes from #1226 + merged #1295

Closed
wants to merge 1 commit into from
Closed

Add Rivets implementation + CR changes from #1226 + merged #1295

wants to merge 1 commit into from

Conversation

altmind
Copy link

@altmind altmind commented May 13, 2015

This is full and exact duplicate of @mikeric implementation in #1226 with following requested changes:

  • removed empty line in html
  • added autofocus on double click on todo
  • resolved merge conflict(deliberately make this separate commit to distinguish new code and merge changes)

New PR as I'm pushing from my personal fork

@altmind altmind mentioned this pull request May 13, 2015
Closed
6 tasks
@samccone
Copy link
Member

jscs is ✅
integration suite needs to be run manually ⚠️

@arthurvr
Copy link
Member

Can we update this to [email protected] and the latest todomvc-common?

@altmind altmind force-pushed the impl-rivets-squashed branch from d5463a4 to be539bf Compare May 18, 2015 10:16
@altmind
Copy link
Author

altmind commented May 18, 2015

@arthurvr done, please review.

@altmind altmind force-pushed the impl-rivets-squashed branch from be539bf to ea58a2e Compare May 18, 2015 15:08
@altmind
Copy link
Author

altmind commented May 19, 2015

Ping.

@samccone
Copy link
Member

@altmind mind squashing this into a single commit?

@arthurvr
Copy link
Member

@altmind You shouldn't start doing ping comments after one day :) We'll be happy to review this but we're all busy :)

Anyways, thanks for working on this! Two other things:

  • The clear completed button is doing weird things. It shows Clear completed () when there are no todos. Take a look at the app spec, we anyways don't require that count anymore.
  • I see 2 failing specs locally. Those sound related to the above, though.

@altmind altmind force-pushed the impl-rivets-squashed branch from ea58a2e to 4de9d22 Compare May 19, 2015 15:35
arthurvr
arthurvr reviewed May 19, 2015
View reviewed changes
examples/rivets/readme.md

* [Guide](http://rivetsjs.com/docs/guide/)
* [Binder Reference](http://rivetsjs.com/docs/reference/)
* [Rivets on Stack Overflow](http://stackoverflow.com/questions/tagged/rivets.js)
Copy link
Member

Choose a reason for hiding this comment

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

Stack overflow and linking up the official site twice is okay but isn't there anything else we can bring up in here? Interesting blogposts maybe?

@altmind altmind force-pushed the impl-rivets-squashed branch 2 times, most recently from b93a164 to bdfe7dd Compare May 20, 2015 09:50
@altmind
Copy link
Author

altmind commented May 26, 2015

Can this be merged already? I addressed and fixed all the comments. It takes weeks just to merge working implementation :(

@sindresorhus
Copy link
Member

We don't get notifications on new commits to a pull request. And please stop the complaining. This is Open Source and we're all doing this in our free time. By opening a pull request you're implicitly asking us to do work (yes, reviewing a pull request takes a lot of work). We'll try to get this merged soon.

@altmind
Copy link
Author

altmind commented May 26, 2015

I just want things to connect and something better to happen. For common benefit. I care.

I am struggling to create implementation for 3 months already #1176

And it always a little bit that is missing. And dangling PRs are slowly decaying(as new requirements require PR changes).

And its demoralizing that i cannot do anything else to help :(

@arthurvr
Copy link
Member

I am struggling to create implementation for 3 months already

Please do note, though, that's mainly because at #1226 the author got inactive and we needed to close it due to inactivity. Anyways, let's move on, I will review all the code again (hopefully this evening) but I'm pretty hopeful that we'll be able to land this soon.

samccone
samccone reviewed May 26, 2015
View reviewed changes
examples/rivets/js/stores/todo-store.js
// Toggles the todo as complete / active and saves.
toggle: function (todo) {
todo.completed = !todo.completed;
this.save(true);
Copy link
Member

Choose a reason for hiding this comment

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

as @sindresorhus pointed out last week to me

http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

@arthurvr
Copy link
Member

What Sam said. For the rest the code looks really good! Thanks for your hard work on this one!

@altmind altmind force-pushed the impl-rivets-squashed branch 2 times, most recently from 364d20c to 8bc2231 Compare May 29, 2015 10:54
@altmind
Copy link
Author

altmind commented May 29, 2015

Fixed #1295 (comment) and #1295 (comment)

Seems like thats it.

arthurvr
arthurvr reviewed May 29, 2015
View reviewed changes
examples/rivets/.gitignore
!node_modules/director/build/director.js

node_modules/rivets/*
!node_modules/rivets/dist/rivets.bundled.min.js
Copy link
Member

Choose a reason for hiding this comment

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

The gitignore looks good but this isn't actually committed.

@altmind altmind force-pushed the impl-rivets-squashed branch from 8bc2231 to 718792e Compare May 29, 2015 15:32
arthurvr
arthurvr reviewed May 30, 2015
View reviewed changes
examples/rivets/js/stores/todo-store.js

localStorage['todos-rivets'] = JSON.stringify(this.all);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

missing semicolon. Please run jshint next time :)

Copy link
Author

Choose a reason for hiding this comment

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

@arthurvr strange, both IDEA and jscs show no jscs errors (using .jscsrc from repo root)

D:\docs\devel\todomvc>jscs -c .jscsrc D:\docs\devel\todomvc\examples\rivets\js\stores\todo-store.js
No code style errors found.

Should I do anything about it?

Copy link
Member

Choose a reason for hiding this comment

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

Also run JSHint ;)

Copy link
Author

Choose a reason for hiding this comment

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

@arthurvr oops. added semicolon :)

@altmind
Add Rivets implementation. Inc changes from tastejs#1226

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
8b0eba9
@altmind altmind force-pushed the impl-rivets-squashed branch from 718792e to 8b0eba9 Compare June 5, 2015 15:20
@arthurvr
Copy link
Member

arthurvr commented Jun 5, 2015

@altmind I will test some more this evening but the code looks good to me. Thanks for the updates.

@arthurvr
Copy link
Member

arthurvr commented Jun 6, 2015

👍 Going to give people some time to complain before landing but this LGTM. Would fix #1180.

@samccone
Copy link
Member

samccone commented Jun 7, 2015

Hi @altmind I just tested this app for memory leaks and it looks like this has a few. ⛲

Steps to reproduce
  • start timeline profiler
    • add three todos
    • delete three todos
    • (repeat above 2 more times)
  • force GC
  • stop timeline profiler

screen shot 2015-06-07 at 12 59 16 pm

screen shot 2015-06-07 at 12 59 59 pm

These should be addressed before we merge

@arthurvr
Copy link
Member

Nice catch @samccone. Ping @altmind about the above.

@arthurvr
Copy link
Member

I'm going to close this due to inactivity. Happy to reopen when you're willing to finish things up. This is sadly the 3th for the rivets example which we need to close due to inactivity.

Can this be merged already? I addressed and fixed all the comments. It takes weeks just to merge working implementation :(

Now you can see yourself why ;)

@arthurvr arthurvr closed this Jun 18, 2015
@arthurvr arthurvr mentioned this pull request Jun 18, 2015
Closed
@blikblum blikblum mentioned this pull request Jan 24, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@altmind @samccone @arthurvr @sindresorhus