fix(checkbox): no side margin if label has no content #2121

belev · 2016-12-08T21:11:11Z

Remove side margin in checkbox if label has no content.
Use checkboxNativeElement instead of checkboxDebugElement.nativeElement in expect statements.
Fix typo.

Closes #2011

tinayuangao · 2016-12-09T09:14:11Z

LGTM

jelbourn · 2016-12-14T03:43:27Z

Could we not move that margin to the label element and then remove it using the :empty pseudo-class? That would be way simpler.

belev · 2016-12-14T14:44:00Z

@jelbourn I totally agree that with such pseudo-class it will be way simpler and better because that's a styling problem. I tried it but couldn't make it work. Using :empty match whitespaces as content and don't apply the changes if we have any whitespace: very simple example. I saw a pseudo-class :blank which handles whitespaces but unfortunately the browsers don't supported it.

belev · 2017-01-05T16:06:59Z

@jelbourn Is there some update about this PR and eventually merging it?

jelbourn · 2017-03-14T22:13:20Z

@belev I want to dedicate a chunk of time to investigate this more in depth but haven't yet had the opportunity. I'll come back here and update when I do.

belev · 2017-03-14T22:52:21Z

@jelbourn Great, looking forward to it.

P.S. sorry for the delayed rebase, I didn't see the changed label.

jelbourn · 2017-04-10T20:15:33Z

src/lib/checkbox/checkbox.scss

@@ -293,6 +300,13 @@ $_mat-checkbox-mark-stroke-size: 2 / 15 * $mat-checkbox-size !default;
        right: $_mat-checkbox-item-spacing;
      }
    }
+
+    &.mat-checkbox-inner-container-no-side-margin {


You don't need to define this class twice; just one without the parent part of the selector would be better.

jelbourn · 2017-04-10T20:16:37Z

src/lib/checkbox/checkbox.ts

@@ -164,6 +164,13 @@ export class MdCheckbox implements ControlValueAccessor, AfterViewInit, OnDestro

  @ViewChild(MdRipple) _ripple: MdRipple;

+  @ViewChild('labelWrapper') _labelWrapper: ElementRef;
+
+  get hasLabel(): boolean {


Let's change this to a method rather than a getter. It should also be marked as internal (name with a leading underscore). It also needs a JsDoc description comment.

jelbourn · 2017-04-10T20:18:48Z

src/lib/checkbox/checkbox.ts

+  @ViewChild('labelWrapper') _labelWrapper: ElementRef;
+
+  get hasLabel(): boolean {
+    const labelWrapperTextContent = this._labelWrapper.nativeElement.textContent;


This will be a problem for Angular Universal. Currently, nativeElement is a parse5 node, which doesn't have a textContent property. So for now you can say

const labelText = this._labelWrapper.nativeElement.textContent || ''; return !!labelText.trim().length;

belev · 2017-04-20T17:30:23Z

I addressed the comments but can't understand why the build fails.

dahaupt · 2017-04-24T12:31:35Z

@belev Probably a Travis bug, happens sometimes as far as I know. Can you restart the CI process?

belev · 2017-04-24T13:36:54Z

@dahaupt I don't think so, doesn't see any restart button. Maybe I can force it by closing and opening the PR or making some new dummy commit but don't think that's a good idea.

devversion · 2017-04-24T14:30:10Z

Yeah looks like a flake. Is your PR rebased properly? Also restarted the build manually.

belev · 2017-04-24T14:34:43Z

What do you mean by properly rebased?

devversion · 2017-04-24T14:35:36Z

I meant rebased on top of master?

belev · 2017-04-24T14:38:29Z

No, it is not.

devversion · 2017-04-24T14:39:40Z

That could have caused the SyntaxError in the Travis CI. Just that you know, rebasing could solve those issues.

belev · 2017-04-24T14:41:36Z

Thanks. So I assume it is better to always rebase on top of master?

devversion · 2017-04-24T14:42:50Z

@belev Yep always :)

belev · 2017-04-24T14:43:41Z

Ok, thanks. I will make the proper rebase when I have time then.

* Remove side margin in checkbox if label has no content. * Use `checkboxNativeElement` instead of `checkboxDebugElement.nativeElement` in expect statements. * Fix typo. Closes angular#2011

belev · 2017-04-25T17:18:54Z

Done, after all 😄

jelbourn

LGTM

w3t · 2017-05-22T17:57:20Z

This change broke the layout of my login screen :(

When rendering:
<md-checkbox [(ngModel)]="savePassword">{{ "tx_Login_SavePassword" | translate }}

It considers that is has no label at render time and therefore renders the checkbox without its margin (until one clicks on the checkbox).

devversion · 2017-05-22T17:58:11Z

@w3t Please file a new issue and provide a Plunker demo for reproduction?

With angular#2121 the margin will be removed for checkboxes that don't have any label set. A problem is that the Checkbox uses the OnPush change detection strategy and therefore the checkbox is not able to detect any delayed / async label change. This means that checkboxes that set their label from an async binding will not have any margin until the users clicks on the checkbox. Using the `cdkObserveContent` seems to be an elegant approach when using the OnPush strategy. The `:empty` CSS selector would be more elegant but it's very sensitive about whitespaces and therefore it doesn't work properly. Fixes angular#4720

* fix(checkbox): margin for empty checkboxes incorrectly added With #2121 the margin will be removed for checkboxes that don't have any label set. A problem is that the Checkbox uses the OnPush change detection strategy and therefore the checkbox is not able to detect any delayed / async label change. This means that checkboxes that set their label from an async binding will not have any margin until the users clicks on the checkbox. Using the `cdkObserveContent` seems to be an elegant approach when using the OnPush strategy. The `:empty` CSS selector would be more elegant but it's very sensitive about whitespaces and therefore it doesn't work properly. Fixes #4720 * Updates

angular-automatic-lock-bot · 2019-09-06T13:01:05Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 8, 2016

belev force-pushed the issue-2011 branch from 5a6f1f2 to 7fd1e12 Compare December 8, 2016 21:27

tinayuangao self-assigned this Dec 9, 2016

jelbourn added the pr: needs review label Dec 14, 2016

andrewseguin added pr: needs rebase and removed pr: needs review labels Mar 2, 2017

belev force-pushed the issue-2011 branch from 2295872 to ab6e01a Compare March 14, 2017 22:41

jelbourn reviewed Apr 10, 2017

View reviewed changes

fix(checkbox): no side margin if label has no content

744505f

* Remove side margin in checkbox if label has no content. * Use `checkboxNativeElement` instead of `checkboxDebugElement.nativeElement` in expect statements. * Fix typo. Closes angular#2011

belev force-pushed the issue-2011 branch from b5507cd to 744505f Compare April 25, 2017 17:05

jelbourn approved these changes Apr 25, 2017

View reviewed changes

jelbourn added the pr: lgtm label Apr 28, 2017

jelbourn added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Apr 28, 2017

mmalerba merged commit 4e8d806 into angular:master Apr 28, 2017

belev deleted the issue-2011 branch April 29, 2017 09:27

w3t mentioned this pull request May 22, 2017

Md-Checkbox has no margin when using dynamic label after #2121 #4720

Closed

devversion mentioned this pull request May 22, 2017

fix(checkbox): margin for empty checkboxes incorrectly added #4730

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

fix(checkbox): no side margin if label has no content #2121

fix(checkbox): no side margin if label has no content #2121

Uh oh!

Conversation

belev commented Dec 8, 2016

Uh oh!

tinayuangao commented Dec 9, 2016

Uh oh!

jelbourn commented Dec 14, 2016

Uh oh!

belev commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

belev commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Mar 14, 2017

Uh oh!

belev commented Mar 14, 2017

Uh oh!

jelbourn Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

jelbourn Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

jelbourn Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

belev commented Apr 20, 2017

Uh oh!

dahaupt commented Apr 24, 2017

Uh oh!

belev commented Apr 24, 2017

Uh oh!

devversion commented Apr 24, 2017

Uh oh!

belev commented Apr 24, 2017

Uh oh!

devversion commented Apr 24, 2017

Uh oh!

belev commented Apr 24, 2017

Uh oh!

devversion commented Apr 24, 2017

Uh oh!

belev commented Apr 24, 2017

Uh oh!

devversion commented Apr 24, 2017

Uh oh!

belev commented Apr 24, 2017

Uh oh!

belev commented Apr 25, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

w3t commented May 22, 2017

Uh oh!

devversion commented May 22, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 6, 2019

Uh oh!

Uh oh!

belev commented Dec 14, 2016 •

edited

Loading

belev commented Jan 5, 2017 •

edited

Loading