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

Partials: Correctly set the HtmlFieldPrefix of the partial view #152

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

fsateler
Copy link
Contributor

Otherwise IdFor and friends do not work as expected because the view prefix is empty.

This also has the advantage of not replacing the user FieldGenerator for the view.

--

Before this patch, if you have the following:

@* Parent.cshtml *@
@using (var f = Html.BeginChameleonForm()) {
     using (var s = f.BeginSection("Fields")) {
           @s.PartialFor(m => m.ChildProperty)
     }
}

@* ChildProperty.cshtml *@

@this.FormSection().FieldFor(c => m.Integer)

<script>console.log($('#@Html.IdFor(m => m.Integer)').length)</script>

The javascript console will print 0, because the id of the Integer field is ChildProperty_Integer but IdFor returned Integer.

After the patch the console will print 1, as expected.

Otherwise IdFor and friends do not work as expected because the view prefix is empty.

This also has the advantage of not replacing the user FieldGenerator for the view.
@fsateler
Copy link
Contributor Author

fsateler commented Dec 7, 2016

ping

This also has the advantage of not replacing the user FieldGenerator for the view.

I just noted that this (replacing the user FieldGenerator) makes this feature unusable for my use case, becase I have several non-default handlers.

@robdmoore
Copy link
Member

This is a good change, but I'm wondering if we need to write a test to cover the bug it's fixing first?

@robdmoore robdmoore merged commit bf0521d into MRCollective:master Jan 28, 2017
@robdmoore
Copy link
Member

I'm doing this as part of some other work I'm doing to fix a bug in Partial.

robdmoore added a commit that referenced this pull request Jan 28, 2017
@fsateler
Copy link
Contributor Author

Indeed, tests would be nice. I'll try to get something and submit it.

@fsateler
Copy link
Contributor Author

fsateler commented Jan 28, 2017 via email

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.

2 participants