search

adamwathan / form

Super basic form HTML builder, only really exists so I can pull it in for some other more useful projects.
MIT License
232 stars 118 forks source link

Add support for autofocus attribute #39

Closed nCrazed closed 9 years ago

nCrazed commented 9 years ago

HTML5 introduces autofocus attribute for text, textarea, select, and button elements: https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Forms_in_HTML#The_autofocus_attribute

This implements a Trait with methods for (un)setting this attribute and applies it to Text, TextArea, Select, and Button classes.

nCrazed commented 9 years ago

Do you want me to address 2 issues highlighted by Scrutnizer (assuming that you want this change at all)?

I originally considered defining the two abstract methods on the trait, but decided against it because that only introduces extra code duplication (if the {set/remove)Attribute signature changes, trait signature will have to change as well), and any errors resulting from the change to the two methods should be caught by the tests anyway.

nCrazed commented 9 years ago

Damn, didn't consider the possibility that you might still be supporting 5.3.

5.3 release has been in end-of life for almost a month now: http://php.net/supported-versions.php, should I bump the version requirement in composer.json?

nCrazed commented 9 years ago

That should've been an amended commit, I'll squash everything once we get passing tests.

adamwathan commented 9 years ago

Hey thanks for the PR! :)

Supporting 5.3 is pretty pointless at this point, I agree :/ Not terribly concerned about bumping it if we need to.

I am a bit torn though between the trait approach or just adding the methods to FormControl like required, enable, etc. I think overall that's simpler, but I'm guessing the reason you didn't do it is because autofocus is not supported for hidden inputs and Hidden here extends FormControl.

I don't think I'm too concerned about that from a "correctness" standpoint honestly, but if you totally disagree I'm happy to hear you out!

nCrazed commented 9 years ago

Actually after, re-reading the MDN article I realise that the 4 elements encapsulate a much larger number of element types (primarily through <input type="$type" />), and hidden is the sole exception (I didn't think it was the case). So mimicking required/enable makes more sense.

That leaves the question of where to put the tests? It looks like required() tests are executed only against Text and omitted for Select, Button, and TextArea, should I do the same here?

adamwathan commented 9 years ago

Re: where to put the tests, that part's a bit messy at the moment you're right, heh. I think for now just slapping them in the Text test and treating that as "the place where we test things defined for everything" is good enough to prevent regressions. Worth figuring out a better way to organize that at some point though, definitely agree.

nCrazed commented 9 years ago

Let me know if you want me to move the tests to TestText until #40 is resolved.

adamwathan commented 9 years ago

Yeah I think that's a good enough plan for now :+1:

adamwathan commented 9 years ago

Thanks again for the PR!