feat(badge): add badge component #7483

amcdnl · 2017-10-02T21:10:13Z

Implements a badge component.

Design Document

https://docs.google.com/document/d/12sksu2GHcITFBidL8nyRzIt8lnVUoGnzwQx4tXWesEw/edit?usp=sharing

ocarreterom · 2017-10-02T22:26:55Z

Nice job, but I think the button implementation breaks the flexibility of the badge component.
I think it's more flexible something like this:

<button mat-button>
  Notifications
  <mat-badge>10</mat-badge>
</button>

-- OR --

<button mat-button>
  Notifications
  <mat-badge content="10"></mat-badge>
</button>

-- OR --

<button mat-button>
  <mat-badge content="10">Notifications</mat-badge>
</button>

Moreover, in the future could be implemented in mat-list, mat-tab, etc.

<mat-tab label="Notifications">
  <mat-badge>10</mat-badge>
</mat-tab>

<mat-list-item>
  Notifications
  <mat-badge>10</mat-badge>
</mat-list-item>

What do you think?

amcdnl · 2017-10-03T13:33:20Z

@jelbourn - Do you think there is a case for just having a bubble with no text in it?

willshowell · 2017-10-03T15:12:44Z

src/lib/badge/badge.ts

+
+  get _isAbove(): boolean { return this.direction.indexOf('above') > -1; }
+
+  get _isBefore(): boolean { return this.direction.indexOf('before') > -1; }


These getters will run every cd cycle, right? What about using a setter in direction instead. Also OnPush for this component?

Good idea :)

willshowell · 2017-10-03T15:20:21Z

src/lib/badge/badge.ts

+  @Input() content: string = '';
+
+  /** Direction the badge should reside; 'before|after above|below' */
+  @Input() direction: string = 'above after';


The default value here is inconsistent with how the _isBefore getter works. If I don't pass a direction, it will end up as above after. If I use "banana banana", it will be below after.

As a nit, the overview guide implies there is an order to which the directions should be in, but that order is reversed in the jsdoc comment.

I made the getters more specific to prevent the monkeys ;P

Thanks for the heads up. Fixed that too.

willshowell · 2017-10-03T15:22:44Z

src/lib/badge/badge.scss

+    align-items: center;
+    position: absolute;
+    font-weight: 600;
+    font-size: 12px;


Should this be set via a typography mixin?

willshowell · 2017-10-03T15:23:21Z

src/lib/badge/badge.ts

+  }
+  private _overlap: boolean = true;
+
+  /** Content of the badget */


willshowell · 2017-10-03T21:42:27Z

src/lib/badge/badge.ts

+  _isAbove: boolean = true;
+  _isBelow: boolean = false;
+  _isBefore: boolean = false;
+  _isAfter: boolean = true;


I don't know if all of these are necessary. In my previous comment, I was thinking more like this

@Input() set direction(val: string) { this._direction = val; this._isAbove = val.indexOf('below') === -1; this._isAfter = val.indexOf('before') === -1; } get direction(): string { return this._direction; } private _direction: string = 'above after'; _isAbove: boolean = true; _isAfter: boolean = true;

That way it always defaults to above unless below is explicitly stated. Likewise for after.

crisbeto · 2017-10-04T17:11:41Z

src/lib/badge/badge.html

+<ng-content></ng-content>
+<span 
+  class="mat-badge-content" 
+  [innerHTML]="content">


This looks dangerous. Is there any reason people would want to put anything besides plain text in a badge?

Removed in latest.

crisbeto · 2017-10-04T17:13:19Z

src/lib/badge/badge.md

+```
+
+### Accessibility
+Badges should be given a meaningful label via `aria-label` or `aria-labelledby` attributes.


I'm not sure whether this is correct. Shouldn't the badge target have an aria-describedby instead @jelbourn?

We need to reconsider how we want to do this with latest api changes.

crisbeto · 2017-10-04T17:19:26Z

src/lib/badge/badge.scss

+  position: relative;
+  display: inline-block;
+
+  &.mat-badge-above {


This nesting seems like it's bumping the specificity unnecessarily. Consider moving it out instead?

I changed this quite a bit, you might want to re-review.

crisbeto · 2017-10-04T17:20:26Z

src/lib/badge/badge.scss

+    display: flex;
+    flex-direction: row;
+    flex-wrap: wrap;
+    justify-content: center;


Does this really need flexbox? It can be done simpler with text-align and line-height.

It now supports font icons and svgs, so its needed now.

crisbeto · 2017-10-04T17:21:08Z

src/lib/badge/badge.spec.ts

+import {MatBadge,  MatBadgeModule} from './index';
+
+
+fdescribe('MatBadge', () => {


Remove the fdescribe.

crisbeto · 2017-10-04T17:22:07Z

src/lib/badge/badge.ts

+    '[class.mat-badge-above]': '_isAbove',
+    '[class.mat-badge-below]': '!_isAbove',
+    '[class.mat-badge-before]': '!_isAfter',
+    '[class.mat-badge-after]': '_isAfter',


I think that you can cut down on the amount of classes by making it appear above before by default. The other classes can override it afterwards.

I started with that but it got odd setting everything backwards. For instance before would have to set right: auto and left: 22px. It felt odd to add that. Happy to change if you guys prefer.

crisbeto · 2017-10-04T17:22:44Z

src/lib/badge/badge.ts

+export const _MatBadgeMixinBase = mixinColor(MatBadgeBase);
+
+@Component({
+  selector: 'mat-badge',


This needs a moduleId: module.id.

Its a directive now so doesn't apply.

crisbeto · 2017-10-04T17:23:29Z

src/lib/badge/badge.ts

+  private _overlap: boolean = true;
+
+  /** Content of the badge */
+  @Input() content: string = '';


Rather than using an input to add the content, why not use content projection? It should allow for move flexibility.

Removed in latest.

amcdnl · 2017-10-05T18:00:20Z

Updated w/ new API. Can you review again @crisbeto @willshowell

willshowell · 2017-10-05T18:50:39Z

src/lib/badge/badge.ts

+
+  /** Injects a span element into the DOM with the content. */
+  private _setContent(): void {
+    let pane = document.createElement('span');


const? here and in _setFontIcon()

willshowell · 2017-10-05T18:51:46Z

src/lib/badge/badge.ts

+  _id: number = nextId++;
+  _isAbove: boolean = true;
+  _isBelow: boolean = false;
+  _isBefore: boolean = false;


_isBelow and _isBefore can be removed I think

willshowell · 2017-10-05T18:58:24Z

src/lib/badge/badge.ts

+
+  /** Theme for the badge */
+  get matBadgeColor(): ThemePalette { return this._color; }
+  set matBadgeColor(value: ThemePalette) {


I don't know if it's a big deal or not, but undefined appears to be invalid for this directive, but is valid in ThemePalette. In form-field, where undefined is also invalid, a custom type is used.

https://github.com/angular/material2/blob/4383f51a58de1fccad2ed64443a5e22ab435c02b/src/lib/form-field/form-field.ts#L97-L100

Hmm, let me know which way you wanna go.

willshowell · 2017-10-05T18:59:57Z

src/lib/badge/badge.md

+typically containing a number or other short set of characters, that appears in proximity to another object.
+
+### Icons
+Badges can contain text or font icons and SVGs registered with the `MdIconRegistry`


willshowell · 2017-10-05T19:00:10Z

src/lib/badge/badge.md

+Badges can contain text or font icons and SVGs registered with the `MdIconRegistry`
+
+#### Font Icons
+Badges using font icons are delcared with the `matIconBadge` tag and its content being


delcared -> declared

willshowell · 2017-10-05T19:03:44Z

src/lib/badge/public_api.ts

+ */
+
+export * from './badge-module';
+export * from './badge';


Rename this file to public-api.ts per #7403

crisbeto · 2017-10-06T15:40:48Z

src/demo-app/badge/badge-demo.html

+    <mat-icon color="primary" matBadge="22" color="accent">home</mat-icon>
+  </button>
+
+  <br />


Use a div with a margin instead of br-s?

crisbeto · 2017-10-06T15:42:08Z

src/demo-app/badge/badge-demo.ts

+  constructor(_iconRegistry: MatIconRegistry, sanitizer: DomSanitizer) {
+    _iconRegistry.addSvgIcon('home',
+      sanitizer.bypassSecurityTrustResourceUrl(
+        'https://upload.wikimedia.org/wikipedia/commons/3/34/Home-icon.svg'));


You can use the home icon from the icon demo instead of fetching one remotely.

I tried that, however, it was not working. That demo page is broken too if you open it up.

crisbeto · 2017-10-06T15:42:56Z

src/lib/badge/_badge-theme.scss

+  position: relative;
+}
+
+.mat-badge:after,


These :after instances should be ::after. I think we had a linting rule for it at some point.

crisbeto · 2017-10-06T15:44:05Z

src/lib/badge/_badge-theme.scss

+.mat-badge-icon,
+.mat-badge-text,
+.mat-badge-svg-icon {
+  position: absolute;


All of these positioning and alignment styles should be in the badge.scss, not in the theme. The theme is only for color-related properties.

I agree, however, since this is a directive there is no way to include styles w/ the component. Do you have a good solution for that?

You can make it a component that has a template of <ng-content>. Having these styles in the theme will cause them to be loaded multiple times for pages with multiple themes.

Ya, thats what I had. You can't do multiple components on the same element though, example: icon w/ a badge.

You can't, but you can have an icon inside a badge.

crisbeto · 2017-10-06T15:44:59Z

src/lib/badge/_badge-theme.scss

+.mat-badge-text,
+.mat-badge .mat-badge-icon {
+  z-index: 2;
+  font-size: 16px;


This should be in the typography mixin and the value, ideally, should come from the typography breakpoints.

Sure, I've moved it inside that mixin.

This size is independent from the font-size of the page since its specifically designed for this fixed width/height of the badge.

crisbeto · 2017-10-06T15:45:09Z

src/lib/badge/_badge-theme.scss

+.mat-badge-svg-icon {
+  z-index: 2;
+  width: 12px;
+  height: 12px;


Move this into a variable?

crisbeto · 2017-10-06T15:47:48Z

src/lib/badge/badge.ts

+
+  /** Clears the previous badge content */
+  _clearContents(): void {
+    const contents = document.getElementById(`badge-content-${this._id}`);


Couldn't you take a hold of the badge content using a ViewChild or ContentChild?

I thought that only worked for angular inserted dom. This is manually inserted.

This plays into my other comment (#7483 (comment)) that it probably shouldn't be constructing DOM in the first place. IMO it should be fine to have it as a component instead of a directive.

Ya, thats what I had but @jelbourn wanted this to be a directive that can decorate components like icons. I totally agree w/ it not constructing dom though, open to suggestions.

You could create a separate component and dynamically compile and attach it (for span or svg, then store it in an array for later removal.

crisbeto · 2017-10-06T15:49:07Z

src/lib/badge/badge.ts

+
+  /** Injects a span element into the DOM with the content. */
+  private _setContent(): void {
+    const content = document.createElement('span');


I don't see why this has to create and insert elements manually. Could you wrap the element via content projection?

As far as I know, directives don't have content projection. You can't have multiple components on the same element either so that would't work. The SVG icons do a similar insertion and removal so this isn't the first place we are doing this type of thing.

I had this as a component at first but that API wasn't what @jelbourn had designed.

crisbeto · 2017-10-06T15:50:42Z

src/lib/badge/badge.ts

+  /**  Gets the font icon set to use. */
+  private _getFontSet(): string {
+    return this.matBadgeFontSet ?
+        this._iconRegistry.classNameForFontAlias(this.matBadgeFontSet) :


This assumes that the consumer will use Material icons, which may not be the case. Why not let them provide the icon element themselves?

This was built to implement Material Icons. I think custom icons might be out of scope for this. @jelbourn - thoughts?

Although I'm curious how you would implement providing the icon element themselves, could you elaborate?

If you convert it into a component and allow content projection, users will be able to override it as they want. Another approach can be to provide an ng-template, but that might be overkill for this case (see #7482).

rafaelss95 · 2017-10-10T23:00:33Z

src/lib/badge/badge-module.ts

+    MatBadge,
+    MatIconBadge,
+    MatSvgIconBadge,
+    MatCommonModule,


Is there any reason to export MatCommonModule here?

Good catch. thanks

rafaelss95 · 2017-10-10T23:05:06Z

src/lib/badge/index.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export * from './public_api';


public-api.

rafaelss95 · 2018-01-21T16:16:47Z

src/demo-app/badge/badge-demo.ts

+  styleUrls: ['badge-demo.css'],
+})
+export class BadgeDemo {
+  visibile = true;


visibile -> visible.

rafaelss95 · 2018-01-21T16:18:16Z

src/demo-app/demo-app/demo-module.ts

@@ -58,6 +58,7 @@ import {TypographyDemo} from '../typography/typography-demo';
 import {DemoApp, Home} from './demo-app';
 import {DEMO_APP_ROUTES} from './routes';
 import {TableDemoModule} from '../table/table-demo-module';
+import { BadgeDemo } from 'badge/badge-demo';


Spaces between braces should be omitted.

rafaelss95 · 2018-01-21T16:18:21Z

src/demo-app/demo-app/routes.ts

@@ -50,11 +50,13 @@ import {TypographyDemo} from '../typography/typography-demo';
 import {DemoApp, Home} from './demo-app';
 import {TableDemoPage} from '../table/table-demo-page';
 import {TABLE_DEMO_ROUTES} from '../table/routes';
+import { BadgeDemo } from 'badge/badge-demo';


Spaces between braces should be omitted.

Stupid editor.

rafaelss95 · 2018-01-21T16:19:55Z

src/lib/badge/badge.ts

+
+let nextId = 0;
+
+export type MatBadgePosition = 'above after' | 'below after' | 'above before' | 'above after';


The above after is duplicated. I think you want to say below before instead?

rafaelss95 · 2018-01-21T16:21:32Z

src/lib/badge/badge.ts

+export class MatBadge {
+
+  /** The color of the badge. Can be `primary`, `accent`, or `warn`. */
+  @Input('matBadgeColor')


Question: The reason that you can't just use the mixinColor here is because the input has an alias?

It needs to have a separate alias for color since the host can also describe a color.

rafaelss95 · 2018-01-21T16:24:29Z

src/lib/badge/badge.ts

+
+  /** Creates the badge element */
+  private _createBadgeElement(): HTMLElement {
+    let badgeElement = this._document.createElement('span');


You could use const here.

amcdnl · 2018-01-21T16:45:34Z

@jelbourn - I think we are good now! * crosses fingers *

rafaelss95 · 2018-01-21T17:02:24Z

@amcdnl It looks like CI isn't happy yet.

jelbourn · 2018-01-22T20:49:56Z

@amcdnl needs update to CODEOWNERS for lint

amcdnl · 2018-01-26T13:27:02Z

@jelbourn - Done :)

jelbourn · 2018-01-31T00:35:33Z

Caretaker note: this will need new build rules

mmalerba · 2018-02-09T17:11:22Z

@amcdnl please rebase

amcdnl · 2018-02-10T15:16:26Z

@mmalerba - Done :)

jagomf · 2018-02-26T10:20:23Z

Is badge not going to be included in 5.2.x but only 6.0?

mmalerba · 2018-02-26T16:06:12Z

@jagomf Yes, it's actually tagged as release: minor, but since there probably won't be another minor release before 6.0.0, it will likely wind up in 6.0.0

angular-automatic-lock-bot · 2019-09-08T10:01:36Z

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.}

feat(badge): add badge component

e3664cc

amcdnl self-assigned this Oct 2, 2017

amcdnl requested a review from jelbourn October 2, 2017 21:10

amcdnl requested a review from devversion as a code owner October 2, 2017 21:10

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 2, 2017

amcdnl removed the request for review from devversion October 2, 2017 21:10

willshowell reviewed Oct 3, 2017

View reviewed changes

amcdnl added 2 commits October 3, 2017 10:56

Merge branch 'master' of https://github.com/angular/material2 into badge

61ca283

chore(nit): address PR feedback

4a454fe

amcdnl requested a review from crisbeto as a code owner October 3, 2017 21:30

amcdnl removed the request for review from crisbeto October 3, 2017 21:31

willshowell reviewed Oct 3, 2017

View reviewed changes

amcdnl added 2 commits October 4, 2017 09:22

chore(nit): address PR feedback

d886d98

Merge branch 'master' of https://github.com/angular/material2 into badge

fb7e717

crisbeto reviewed Oct 4, 2017

View reviewed changes

amcdnl added 2 commits October 5, 2017 12:45

feat(badges): rewrite w/ new api

c0ecf62

feat(badge): update a few css classes

1640a5c

amcdnl requested a review from crisbeto October 5, 2017 18:01

willshowell reviewed Oct 5, 2017

View reviewed changes

chore(nit): fix nit feedback

a7077d9

crisbeto reviewed Oct 6, 2017

View reviewed changes

chore(nit): update per PR feedback

d1adb94

rafaelss95 reviewed Oct 10, 2017

View reviewed changes

amcdnl added 3 commits October 11, 2017 09:25

chore(nit): update per feedback

d3e4e50

Merge branch 'master' of https://github.com/angular/material2 into badge

7a82eba

chore(nit): updates post review

ec10e44

rafaelss95 reviewed Jan 21, 2018

View reviewed changes

chore: pr feedback

6c6852d

chore: fix lint

7b40790

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release labels Jan 31, 2018

mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 9, 2018

Merge branch 'master' into badge

d60e190

amcdnl removed the pr: needs rebase label Feb 10, 2018

mmalerba added the action: merge The PR is ready for merge by the caretaker label Feb 10, 2018

jelbourn merged commit c98d217 into angular:master Feb 13, 2018

amcdnl deleted the badge branch February 17, 2018 17:17

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


		get _isAbove(): boolean { return this.direction.indexOf('above') > -1; }

		get _isBefore(): boolean { return this.direction.indexOf('before') > -1; }

		import {MatBadge, MatBadgeModule} from './index';


		fdescribe('MatBadge', () => {


		let nextId = 0;

		export type MatBadgePosition = 'above after' \| 'below after' \| 'above before' \| 'above after';

feat(badge): add badge component #7483

feat(badge): add badge component #7483

Conversation

amcdnl commented Oct 2, 2017 • edited Loading

Design Document

ocarreterom commented Oct 2, 2017

amcdnl commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcdnl commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcdnl Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcdnl commented Jan 21, 2018

rafaelss95 commented Jan 21, 2018

jelbourn commented Jan 22, 2018

amcdnl commented Jan 26, 2018

jelbourn commented Jan 31, 2018

mmalerba commented Feb 9, 2018

amcdnl commented Feb 10, 2018

jagomf commented Feb 26, 2018

mmalerba commented Feb 26, 2018

amcdnl commented Oct 2, 2017 •

edited

Loading

amcdnl Oct 11, 2017 •

edited

Loading