-
Notifications
You must be signed in to change notification settings - Fork 105
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
Allow customization of default wrapper in order to support Bootstrap without requiring templates #349
Conversation
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.
All minor/trivial comments.
Tests for the new code are missing.
grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy
Outdated
Show resolved
Hide resolved
if (k?.startsWith(prefixAttribute)) | ||
widgetAttrs[k.replace(prefixAttribute, '')] = v | ||
else | ||
wrapperAttrs[k] = v | ||
} | ||
|
||
List classes = [widgetAttrs['class']?:''] | ||
if (model.invalid) classes << (widgetAttrs.remove('invalidClass')?:'') | ||
if (model.required) classes << (widgetAttrs.remove('requiredClass')?:'') |
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.
To follow the code-style (if any) of the existing code, add spaces around ?:
(throughout changes)
grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy
Outdated
Show resolved
Hide resolved
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.
All good
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.
Adding a few tests to exercise the new functionality would ensure it doesn't regress.
@jamesfredley existing tests should verify functionality because all the new attributes have existing defaults. If the code changed any existing behavior, the unit tests would fail. I am a little hesitant to add a unit tests for any new capabilities because, with consensus from everyone, I would like to completely change the default behavior for 7.0. |
@codeconsole ok, if it is all going to change, the test would be short lived and unneeded. |
This Objective
Reduce the amount of code in generate-app to the least amount possible while still maintaining readability and structure.
It is here for review and feedback
Design Decisions:
<li><label></label><div>widget</div></li>
pattern is needed. To maintain backwards compatibility, the div is only injected if thedivClass
attribute is present.