MRCollective / ChameleonForms

Shape-shifting your forms experience in ASP.NET Core MVC
MIT License
254 stars 56 forks source link

Append/Prepend overloads that take a lambda #69

Open becdetat opened 10 years ago

becdetat commented 10 years ago

So I could do:

@s.FieldFor(x => x.Email).Append(() => {
    if (Model.Email == "") return "";
    return "<a href='mailto'/.......
})

without having to make a helper every time or do cray-cray inline logic.

MattDavies commented 10 years ago

I think it might be more useful if the action gave you the property in question? ie:

@s.FieldFor(x => x.Email).Append(e => { if (e == "") return ""; return "<a href='mailto'/....... })

but otherwise I would probably just use a ternary in this situation, given the extra overload doesn't really make it any cleaner than this:

@s.FieldFor(x => x.Email).Append( Model.Email == "" ? "" : "<a href='mailto'/......."; )

Thoughts?

becdetat commented 10 years ago

Giving the property is a good idea. My idea was that having the overload would allow arbitrary code, not just ternaries. Since this is the only way to inject markup for a field I would like the extra control.

MattDavies commented 10 years ago

Sure - with the property this would be really cool then. @robdmoore might have other thoughts

robdmoore commented 10 years ago

Lambdas and views don't play nicely together. I'd say @helper or complex code in view model is nicer tbh.

Unless there is a second overload that takes a Boolean property for whether or not to display it.

becdetat commented 10 years ago

Why don't views like lambdas? They're cute and cuddly. Everybody likes lambdas.

I was sort of inspired by jQuery's append() which takes an anonymous function. It's kinda just sugar but I like my sugar like I like my lambdas. In my views. I should stop.

I would agree with extracting it out into the viewmodel, not so much into a helper for a single case as it moves the declaration of a field (@s.FieldFor()) away from its implementation (the helper). But I would like to start out with a lambda and extract that out if the logic warrants it, example would be once it becomes complex enough to warrant unit testing. This syntax feels like a natural middle ground.

I don't think the boolean property would be very discoverable. maybe something like

@s.FieldFor(x => x.Foo).Append(x => ,,,).If(x => x.Foo != null)
robdmoore commented 10 years ago

Basically two-fold: sometimes I've noticed multi-line constructs in views become really nasty or difficult (admittedly, I'm more thinking about fluent insterfaces; lambdas might be OK.

Also, the formatting and indentation of ReSharper and VS in razor views still leaves a lot to be desired.

So possibly it's not such a bad idea.

Re: moving out IHtmlString definitions to helpers I actually like that it moves it away from the field definition, but for the case you describe above I can see the value in keeping it with the field definition since it's small enough that you loose context in moving it away.

The .If thing reads cool, but isn't possible to implement and having a second level of fluent interface is confusing regardless (plus it will make @graemefoster cry :().

Let's run with an append that takes the Model itself as a parameter? I think that's more useful than the property itself and more consistent with all the other lambdas.

i.e.:

@s.FieldFor(x => x.Email).Append(m => {
    if (string.IsNullOrEmpty(m.Email))
        return new HtmlString("");
    return new HtmlString(string.Format("<a href=\"mailto:{0}\">{0}</a>", m.Email.ToHtml()));
})

Although I'm still leaning towards a helper or view model abstraction being cleaner given you have to wrap in the HtmlString (which both of your examples were missing).