MRCollective / ChameleonForms

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

Rename WithoutLabel to WithoutLabelElement and deprecate it #122

Closed zabulus closed 9 years ago

zabulus commented 9 years ago

Here is a test in DefaultFieldGeneratorTest that fails:

        [Test]
        public void GetLabelHtml_should_not_return_any_string_if_WithoutLabel_used_and_DisplayAttribute_present()
        {
            var generator = Arrange(x => x.StringWithDisplayAttribute);
            var fieldConfig = new FieldConfiguration();
            fieldConfig.WithoutLabel();
            var config = generator.PrepareFieldConfiguration(fieldConfig, FieldParent.Section);
            var actual = generator.GetLabelHtml(config).ToString();
            Assert.That(actual, Is.Empty);
        }
robdmoore commented 9 years ago

I think this is a misunderstanding maybe? .WithoutLabel means don't use the label element rather than don't show a label at all?

zabulus commented 9 years ago

Hm that makes sense. Maybe it should be named WithoutLabelTag? Or maybe add clarification to the documentation. BTW I thought field configuration should specify what generate and not how generate.

robdmoore commented 9 years ago

I agree that WithoutLabel is misleading. The xmldoc should make it clear what it does.

I'm hesitant to make a breaking rename change because this code is called from razor views which most people don't compile. We could create an alias name that is easier to understand though (but simply calls WithoutLabel). We could even mark WithoutLabel obsolete so people don't use it from now on?

As to what the method should be called. What about WithoutLabelElement?

zabulus commented 9 years ago

Fix according to last comment