Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless#24728
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
arkste commentedOct 30, 2017
I've added tests to make sure the checkbox row doesn't break again in bootstrap 3 & 4. TwigBridge tests were failing because of the changes made in#24703, so i fixed them as well. Tests are still failing, but this is related due to the timezone changes. Please let me know if there's anything else i can do to get this merged. |
| {%spaceless %} | ||
| {%iflabelissame as(false) %} | ||
| <divclass="{{block('form_label_class') }}"></div> | ||
| {%else %} |
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.
these tags are missing the whitespace control to remove indentation spaces
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.
changed. please review the recent commit, thanks!
arkste commentedOct 30, 2017
Status: Needs Review |
arkste commentedOct 30, 2017
@stof i've added more whitespace control tags to remove indentation spaces. |
nicolas-grekas commentedOct 30, 2017
deps=low tests should not fail: Form's lowest dep for the bridge should be bumped (or vice-versa, didn't look precisely) |
arkste commentedOct 30, 2017
@nicolas-grekas not sure what to do now, the deps are already on |
nicolas-grekas commentedOct 30, 2017
should be |
arkste commentedOct 30, 2017
correct me if i'm wrong, but even changing the deps wont make the tests running green on deps=low, since travis runs the bridge tests from the vendor-folder (like |
nicolas-grekas commentedOct 30, 2017
it will: the CI builds per PR packages for that situation |
| {%blockform_label_class -%} | ||
| col-sm-2 | ||
| {%-endblockform_label_class %} |
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.
Can you move back this snippet to where it was before? That would ease merging older branches. Thanks.
| {%blockform_group_class -%} | ||
| col-sm-10 | ||
| {%-endblockform_group_class %} |
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.
Same for this one. We try to not move snippets of code around as it makes merging branches more difficult.
arkste commentedOct 30, 2017
@fabpot i reverted the change for bootstrap 3 & 4. |
| {%blockcheckbox_widget -%} | ||
| {%-setparent_label_class=parent_label_class|default(label_attr.class|default('')) -%} | ||
| {%if'checkbox-inline'inparent_label_class %} | ||
| {%-if'checkbox-inline'inparent_label_class-%} |
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.
Are you sure those added- are needed? Does they make a difference as there are already- signs at the end of the line before and at the beginning of the next line?
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.
actually they're not needed, but i tried to keep it similar to the bs4 layout, will remove them in both templates.
nicolas-grekas left a comment
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.
the diff should be reduced to the strict minimum (there are some added "-" that are not needed)
| {{- form_errors(form) -}} | ||
| </div>{#--#} | ||
| </div> | ||
| {%-endblockcheckbox_row %} |
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.
missing end of line
fabpot commentedOct 30, 2017
Thank you@arkste. |
…y & remove spaceless (arkste)This PR was squashed before being merged into the 3.4 branch (closes#24728).Discussion----------[Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24711| License | MIT| Doc PR | -As discussed in#24711 i reverted the change i did in `bootstrap_3_layout.html.twig` (which caused an unnecessary empty div-container in the vertical-layout), added the `checkbox_row` block to the `bootstrap_3_horizontal_layout.html.twig` and removed `spaceless` (as proposed in#24727).since i added `{#--#}` in bootstrap 3, i did the same for the same horizontal blocks in bootstrap 4 as well.I moved the `form_label_class` & `form_group_class` blocks to the top of `bootstrap_3_horizontal_layout.html.twig` & `bootstrap_4_horizontal_layout.html.twig`, this should improve DX as they were spreaded across the file.#24702 affected the bootstrap 4 horizontal layout as well, so i added the `checkbox_row` block to bootstrap 4 too.ping@fabpot@nicolas-grekasCommits-------f84749f [Bridge\Twig] fix bootstrap checkbox_row to render properly & remove spaceless
arkste commentedOct 30, 2017
seems like i pushed the EOL's while this was supposed to be merged. anything i should do now@fabpot ? |
fabpot commentedOct 30, 2017
@arkste all is fine :) Thank you. |
arkste commentedOct 30, 2017
ok :) |
Uh oh!
There was an error while loading.Please reload this page.
As discussed in#24711 i reverted the change i did in
bootstrap_3_layout.html.twig(which caused an unnecessary empty div-container in the vertical-layout), added thecheckbox_rowblock to thebootstrap_3_horizontal_layout.html.twigand removedspaceless(as proposed in#24727).since i added
{#--#}in bootstrap 3, i did the same for the same horizontal blocks in bootstrap 4 as well.I moved the
form_label_class&form_group_classblocks to the top ofbootstrap_3_horizontal_layout.html.twig&bootstrap_4_horizontal_layout.html.twig, this should improve DX as they were spreaded across the file.#24702 affected the bootstrap 4 horizontal layout as well, so i added the
checkbox_rowblock to bootstrap 4 too.ping@fabpot@nicolas-grekas