-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form][TwigBridge] Add help_attr #27043
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
Conversation
@@ -1,3 +1,4 @@ | |||
<?php if (!empty($help)): ?> | |||
<p id="<?php echo $view->escape($id); ?>_help" class="help-text"><?php echo $view->escape(false !== $translation_domain ? $view['translator']->trans($help, array(), $translation_domain) : $help); ?></p> | |||
<?php $help_attr['class'] = trim((isset($help_attr['class']) ? $help_attr['class'].' help-text' : 'help-text')); ?> |
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.
you could apply the trimming around the concatenation only, as the other alternative is already trimmed.
Is this still needed given that #26332 was merged? |
This follow the last, this PR add a 'help_attr', like 'label_attr' for exemple. That permit to configure the 'help' entry. |
@mpiot LGTM, can you please rebase here? |
@xabbuh yes, I do that on Monday ;-) And I'll see to add a part about that in the doc |
We need to update the constraints for the Form component in the |
Mmm, sorry I don't really know what to do :-/ I replace all 4.1 per ... ? 4.2 ? But I need to declare somewhere the version change ? And I need to do the same in the Twig Bridge ? I'm so sorry, I don't really know how it works. |
@xabbuh Thanks I try to understand errors with deps=high. On my machine when I do something like:
And tests are OK, I don't see the difference with Travis, because I use the same commands as in the travis test. Edit: I try to add the form version requirement in the FrameworkBundle too |
@mpiot This is a bit tricky. We use Travis to ensure that 4.1 versions of components work with 4.2 dependencies. The form theme tests are not very flexible as they mainly just compare the rendered HTML. I try to find a way to relax them a bit. |
I've not reviewed this PR but I've spotted a merge commit in the history. @mpiot Can you rebase to get rid of it (as PR with merge commits cannot be merged)? Thank you. |
@xabbuh thanks, so there's no way to pass the tests right now? |
@mpiot I try to look into it tonight. |
If I don't miss anything here, tests would pass just when we merge this into the |
Thank you for the time you spent watching this :-) I'll write the doc asap :-) |
Thank you @mpiot. |
This PR was merged into the 4.2-dev branch. Discussion ---------- [Form][TwigBridge] Add help_attr | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Add help_attr to the form_help method. Commits ------- 42d54b7 Add help_attr
@mpiot FYI, @nicolas-grekas helped me to spot why the tests where failing (I moved the test method from the Form component to the concrete subclasses in FrameworkBundle and TwigBridge to fix it). |
@xabbuh Thank you for this feedback, always important to know what was wrong :-) |
This PR was squashed before being merged into the master branch (closes #10511). Discussion ---------- [FormType] Add help_attr documentation Add documentation about the new feature symfony/symfony#27043 Commits ------- f6162bc [FormType] Add help_attr documentation
Add help_attr to the form_help method.