Fix for Issue #1384 #1391

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

Contributor

In this fix for #1384 , I tried to stay close to Mapper implementation.

Not sure if a little test I made makes much sense.

Contributor
nafg commented Jan 7, 2013

Looks very good.
My only question is, maybe it should be a type-safe enum, i.e., a series of case objects for all the valid values.

sealed class InputType(val value: String)
case object Text extends InputType("text")
case object Number extends InputType("number")

And write type={formInputType.value}

Perhaps it should include everything, like hidden, checkbox, what about submit...

Owner

@nafg I think you're over thinking it. The idea is to make it more flexible, and it's necessary because HTML5 has expanded the number of types available. If we restrict it, I can only see it causing problems.

+1 for the fix as is. My only hesitation is that this is a bit more code than I've seen from a non-committer (granted, I don't review everything). I don't see anything here that I could imagine as an IP issue though, so I'm ok with it unless someone else disagrees.

Contributor

I personally like the idea of type-safe input type, but then it becomes even more apparent that this attribute is used only in a few Field classes. So maybe it would be more appropriate to move this formInputType to a new trait that we could add only to some of the field classes. Something like:

trait Html5Input {
def formInputType = "text" //or the type-safe version
}

I might be over thinking it too, but it bothers me a little that overriding BooleanField's formInputType would not have any effect, and you could override StringField to become a checkbox.

As too the IP problem - the current solution is a copy of the Mapper's solution, so I don't think I could ever claim the ownership.

Contributor
nafg commented Jan 7, 2013

I'm not concerned about the size. Most of it is in the tests.
Also there's a difference between 23 lines of a complicated algorithm, and something simple like this. I think this is exactly the kind of thing that it's good to "outsource" to the community.

Owner

I also think that the amount of code is still ok for a non committer, so, how about we rebase this to master and if you guys want a second version, more type safe, we create a new pull request? (I'd like to get a few pull request into master so we can build the next milestone of Lift (with 2.10 support ) and a few other fixes.

Owner
d6y commented Jan 8, 2013

+1

I think the size is fine too. In fact, it might be possible to make it larger by including the change on field/EmailField.scala for type "email" --- but that's just speculation! I've not looked at how that renders everywhere - so just a comment, no need to act on it.

Owner

👍 Merge it!

Contributor

@d6y - from what I can see, EmailField extends StringField so it's perfectly ok to do:

override def formInputType = "email"

and it should work. Unless you meant actually putting such code into EmailField. But then to be consistent we should override this field to "number" in NumericField, "date"/"datetime" in DateTimeField and such. But I'm not sure if most people using Record expect these fields to be HTML5 specific.

Owner
d6y commented Jan 8, 2013

@jczuchnowski I did mean to default it to formInputType "email". But yes, that requires more discussion, so we should forget about it for now.

Owner

ok, tonight is merge night, I'll merge this and the other 2 pending pull requests

Owner

Rebased to master, this change will be on sonatype in a few hours (for 2.5-SNAPSHOT)

Thanks for the contribution!

@fmpwizard fmpwizard closed this Jan 10, 2013
Contributor

Its' my first contribution, but hopefully not the last. Thanks for Lift !

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