-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Adapted basic form elements to V5 #167
Conversation
syberon
commented
May 26, 2021
- Class 'form-group' replaced by 'mb-3'
- Class 'form-label' added to LABEL tag
- Checkbox element adapted to V5
- Select element adated to V5
- Radio element adapted to V5
- File element adapted to V5
- Removed 'custom' option support for elements - it's not supported by V5 anymore
@@ -105,7 +105,7 @@ public function render(\Laminas\Form\ElementInterface $oElement, $sLabelPosition | |||
*/ | |||
public function renderFormRow(\Laminas\Form\ElementInterface $oElement, $sElementContent): string | |||
{ | |||
$aRowClasses = ['form-group']; | |||
$aRowClasses = ['mb-3']; |
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.
Spacing should not be hardcoded.
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.
This spacing is according the official documentation because without this hardcoded spacing we'll need to add the additional class to every element in form factory. In V4 this spacing is added vie form-group class but in V5 this class is removed and all controls renders to close to each other.
Maybe we can add an option to config array to define the default class for spacing?
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.
This spacing is according the official documentation…
This is only an example in the documentation and does not mean that every form must use the same spacing.
Maybe we can add an option to config array to define the default class for spacing?
I think this is the correct direction.
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.
I think this is the correct direction.
I made this change in new commit.
1. Removed formFile custom helper because there is not needed anymore. 2. Added option defaultRowSpacingClass to config array to define default class for row render. 3. Removed 'file' helper from ignoredViewHelpers to add a default 'form-control' class to this helper by default.
@neilime, all, thank you for your great work here! One thing I am wondering though is how you plan to release support for Bootstrap 5. With the upcomming release of lamians-form 3.0.0, there will be breaking changes - as the signatures for View Helpers have changed (see laminas/laminas-form#128), you will likely need to release a new major version of twbs-helper-module, as adapting to these changes of laminas-form v3 would be a BC break for this library. Lets say that tbws-helper-module 4.x features laminas-form 3.x with Boostrap 4. Would your plan then be to release twbs-helper-module 5.x for Boostrap 5 support? That would result in the need to maintain at least two major versions, probably even three for some time, if support for laminas-form 2.x should be continued. I am wondering if it would make more sense to add support for Boostrap 4 and 5 through configuration somehow... |
@driehle the first milestone is to migrate to Bootstrap 5, when this will be stable, a new tag will created incrementing major digit (probably 4.0.0) Then the next focus will be on laminas Form. At this moment every good ideas for providing backward compatible feature (Bootstrap v4 / v5, form v2 / v3) will be good to be shared. |