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
Foundation 5 Adaptation &co. #8
Conversation
- Password field uses input() function that is teased in the Email field tests - SelectMonth and SelectRange are unchanged and they both use select() which is tested in its own tests
- Textbox field uses input() function that is teased in the Email field tests
- Added the new ``withLabel($name, $label)`` that allows to define a label for a form item. - Label that are added using this new method will wrap around the element as suggested by Foundation 5 [http://foundation.zurb.com/docs/components/forms.html] - Checkboxes and Radio that are going to use this new method will have a specific ID so the labels can be correctly clicked to check the input item. - Tests have been improved using ``assertTag()`` instead of an intricate and unsafe ``assertTrue()`` using ``strpos``.
This is pretty cool. I will take a closer look at this over the weekend and take action. Thank you for contributing! |
Ok, if you'll have any question just ask me. :-)
Sent from mobile.
|
'tag' => 'input', | ||
'attributes' => ['type' => 'email'] | ||
]; | ||
$this->assertTag( $expected_tag, $result ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for porting all the tests to use AssetTag. This makes a lot of sense.
I didn't take a look of the full code changes but I saw there is a new method I hope it doesn't because so far this package was a drop-in replacement for the official Laravel FormBuilder. If anyone starts using the |
I agree with @Stolz here. Part of my immediate hesitation is that this is adding new functionality that I am still deliberating on it's value here. I will continue to deliberate. |
No it does not, |
@esolitos Thank you for the contributions. I appreciate the thought and time you have put into your solution. I really like your ideas and feedback regarding the tag assertions. I feel that @Stolz's point regarding the "withLabel" is a valid one, in that the solution is meant to be invisible to the normal Laravel development process. If this package promises new functionality, it could cause some individuals a great deal of headache should they decide to no longer use this package in their project. I do like what you are proposing and would encourage you to contribute your solution to the laravel/framework repository. If @taylorotwell et al agree to merge it, I think it will be a valuable addition to the FormBuilder class. I took note of some of your changes to add support for the checkable elements. I have been reluctant to move forward with support for those as displaying validation messages for checkboxes and radio buttons, within and outside of a group, as it is a bit tricky. I am interested to hear more about your ideas on how you would go about this. Would you be interested in opening a new pull request that focused solely on that feature enhancement? Lastly, I did just push an update to the package that implements the tag assertions as you recommended. |
Hi, I made some improvements to this nice package. Mostly with the idea to port it to Foundation 5.
Those are the changes:
withLabel($name, $label)
that allows to define a label for aform item.
by Foundation 5 [http://foundation.zurb.com/docs/components/forms.html]
ID so the labels can be correctly clicked to check the input item.
assertTag()
instead of an intricate and unsafeassertTrue()
usingstrpos
.Taking the README example for the Usage chapter here's how it simplifies:
will render without errors
and in case of errors a
<span class="error">$error_message</small>
will be added inside the label and the classerror
will be applied to both the<label>
and theinput/select/textarea/...
.The method
withLabel()
can also be used to define all the labels like:and I'm planning to allow to accept an associative array as parameter or to add a
defineLabels()
method to define all the labels at once (but i'm not not sure how to manage checkboxes).Let me know what do you think about it.
In case you don't want to merge I'll make a separate package keeping you in the credits as original author.
Note: If you look at the full diff of
FormBuilder
it is "complex" to understand, look at the single commits and the steps are going to be more clear.PS: This is the laravel view I use for testing the output: