Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Serversite error handling, rails "compatible" validatotors, new event callbacks, validator generation from rails and more #7

Closed
wants to merge 57 commits into
from

Conversation

Projects
None yet
3 participants
Collaborator

simcha commented Jan 21, 2014

Dmytrii,
My colleague Krzysztof Madejski has upgraded your nice tool for use in our project. I have rewritten specs to mirror the changes. I would like to ask you to have a look. Merge or give me some guidelines as what to change.
Changes are described in the README.md
Thanks in advance and thanks for knockout-rails.
Jan

Krzysztof Madejski and others added some commits Feb 27, 2013

@dnagir dnagir commented on the diff Jan 23, 2014

@@ -7,10 +7,8 @@ group :development, :test do
gem 'pry'
gem 'jasminerice'
+ gem 'sqlite3'
@dnagir

dnagir Jan 23, 2014

Owner

Not sure why you need sqlite3 here. You can always use ActiveModel, no need for a full-on database when testing client side JS.

@dnagir dnagir commented on the diff Jan 23, 2014

README.md
@@ -189,7 +211,7 @@ class @Page extends ko.Model
class @Page extends ko.Model
@persistAt 'page'
- @on 'beforeSave', ->
+ @upon 'beforeSave', ->
@dnagir

dnagir Jan 23, 2014

Owner

What's the reason for a breaking change from on to upon?

@KrzysztofMadejski

KrzysztofMadejski Jan 24, 2014

Because there was never an 'on' function. It's an error in the example.

@dnagir dnagir commented on the diff Jan 23, 2014

lib/assets/javascripts/knockout/ie_hack.js
+ },
+ set: function (value) {
+ }
+ });
+}
+
+// console.log hack
+if (!window['console']) {
+ window.console = {};
+}
+var console_methods = ["log", "warn", "info", "debug"];
+var method;
+for (var i = 0; i < console_methods.length; ++i) {
+ method = console_methods[i]
+ if (!window['console'][method]) {
+ window.console[method] = function () {
@dnagir

dnagir Jan 23, 2014

Owner

Don't think it is a good idea to patch global browser objects. Just saying.

@simcha

simcha Jan 24, 2014

Collaborator

Sure I will remove it.

@dnagir dnagir commented on the diff Jan 23, 2014

lib/assets/javascripts/knockout/bindings/onoff.js.coffee
- initialValue = ko.utils.unwrapObservable valueAccessor()
- $(element).prop('checked', initialValue).iphoneStyle
- handleMargin: 0
- containerRadius: 0
- resizeHandle: false
- resizeContainer: false
- checkedLabel: 'On'
- uncheckedLabel: 'Off'
- onChange: (el, checked) ->
- writer = valueAccessor()
- writer checked
-
- update: (element, valueAccessor) ->
- el = $(element)
- val = ko.utils.unwrapObservable valueAccessor()
- el.prop('checked', val)
@dnagir

dnagir Jan 23, 2014

Owner

Why did you drop all the existing bindings?

@simcha

simcha Jan 28, 2014

Collaborator

We were thinking to spin bindings to separate project so people can choose what bindings they like and we will free project from dependencies. But we didn't finish it we should do ether this or that. What you think?

@dnagir

dnagir Jan 28, 2014

Owner

@simcha yes, moving those bindings to a separate project is a good idea.

Owner

dnagir commented Jan 23, 2014

I don't know. This is a bit too large PR to merge especially taking into account number of breaking changes dropped bindings and extensions introduced.

But since I'm not maintaining this repo I shouldn't be complaining. Unless someone wants to take over this repo and proper maintenance I'd probably prefer you to keep this work in your fork.

Collaborator

simcha commented Jan 28, 2014

Dmytrii, we find your work very valuable and we use knockout-rails in 2 of our (in4mates) projects. Thanks for reviewing our work. If you are not planing to maintain the repo and there is no one that is willing to do it, I would volunteer to do it. First of course I would need to fix errors you found and rewrite read.me. I will of course understand if you prefer me not to maintain the repo. Thanks again
Jan

@simcha simcha closed this Jan 28, 2014

Owner

dnagir commented Jan 28, 2014

@simcha I'll add you as the contributor to this repo.
But be a little careful as I won't be able keep a close eye on it.

Good luck with it and thanks.

Owner

dnagir commented Jan 28, 2014

Now @simcha and @jaredjenkins are collaborators on this repo. Please coordinate your actions together.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment