feat(expansion-panel): initial version of expansion panel #4191

josephperrott · 2017-04-21T07:19:49Z

crisbeto · 2017-04-21T16:50:52Z

src/lib/expansion-panel/expansion-panel.html

@@ -0,0 +1,6 @@
+<ng-content select="md-summary"></ng-content>


This one also needs to handle mat-summary.

crisbeto · 2017-04-21T16:51:13Z

src/lib/expansion-panel/expansion-panel.html

@@ -0,0 +1,6 @@
+<ng-content select="md-summary"></ng-content>
+<div class="md-expansion-panel-body" [class.expanded]="expanded"


The expanded class needs to be prefixed (e.g. mat-expanded).

crisbeto · 2017-04-21T16:51:40Z

src/lib/expansion-panel/expansion-panel.html

+<div class="md-expansion-panel-body" [class.expanded]="expanded"
+     [@bodyExpansion]="booleanToString(expanded)" [id]="getExpansionPanelId()">
+  <ng-content></ng-content>
+  <ng-content select="[md-action-row]"></ng-content>


This should also support the mat-prefix.

crisbeto · 2017-04-21T16:52:50Z

src/lib/expansion-panel/expansion-panel.scss

+  display: block;
+
+  &:not(:first-child) {
+    border-top: 1px solid rgba(0, 0, 0, 0.12);


The color here and on line 33 should come from the theme.

crisbeto · 2017-04-21T16:54:24Z

src/lib/expansion-panel/expansion-panel.scss

+    @include mat-elevation(2);
+  }
+
+  &[wide='true'] {


Perhaps this should be done through a class? Also it won't work if wide was used as a boolean attribute (e.g. <md-expansion-panel wide>.

crisbeto · 2017-04-21T17:06:47Z

src/lib/expansion-panel/expansion-panel.ts

+    return this.panelSet && this.panelSet.style === 'wide';
+  }
+  /** Whether the expansion panel is expanded. */
+  private _expanded: boolean;


Might be a good idea to add a default value to this one.

crisbeto · 2017-04-21T17:07:35Z

src/lib/expansion-panel/expansion-summary.scss

+  line-height: $md-summary-height;
+  padding: 0 24px;
+
+  .content {


This class should be prefixed with mat- and not be nested.

crisbeto · 2017-04-21T17:07:51Z

src/lib/expansion-panel/expansion-summary.scss

+
+  &:focus, 
+  &:hover {
+    background: rgb(238, 238, 238);


The color should be inside the theme. The same goes for [title], [description] etc.

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

src/lib/expansion-panel/expansion-summary.scss

+
+  &.expanded:focus,
+  &.expanded:hover, {
+    background: initial;


AFAIK initial doesn't work on IE.

crisbeto · 2017-04-21T17:09:37Z

src/lib/expansion-panel/expansion-summary.ts

+    '[attr.aria-expanded]': 'parentPanel.expanded',
+    '[class.mat-summary]': 'true',
+    '[class.expanded]': 'parentPanel.expanded',
+    '(click)': 'parentPanel.expanded = !parentPanel.expanded',


It would be better if these expressions were in a method inside the class, instead of inline.

Throws a Stylelint error when using the `initial` value for a property. `initial` doesn't work in IE and is easy to work around by looking up the default value for the property. Related to angular#4191 (comment).

jelbourn

Finished first pass, also agree w/ all of @crisbeto's comments.

jelbourn · 2017-04-21T18:13:02Z

src/demo-app/expansion-panel/expansion-panel-demo.html

@@ -0,0 +1,54 @@
+<h1>Single Expansion Panel</h1>
+<md-expansion-panel class="md-expansion-demo-width" #myPanel>


Can we rename this to <md-details> (and update all surrounding text)? I know that the spec calls them "expansion panel", but I'd prefer to stick closer to the native API where possible

(e.g., the spec refers to "select" as "dropdown button" and to inputs as "text field")

jelbourn · 2017-04-21T20:08:14Z

src/lib/expansion-panel/expansion-panel.scss

+  box-sizing: content-box;
+  display: block;
+
+  &:not(:first-child) {


.mat-expansion-panel could be the first child of an arbitrary div

jelbourn · 2017-04-21T20:08:32Z

src/lib/expansion-panel/expansion-panel.scss

+  }
+}
+
+div.md-expansion-panel-body {


Why is div part of this selector?

jelbourn · 2017-04-21T20:10:10Z

src/lib/expansion-panel/expansion-panel.ts

+@Directive({
+  selector: 'md-expansion-panel-set, mat-expansion-panel-set',
+  host: {
+    '[class.mat-expansion-panel-set]': 'true',


You can just use a static class here, e.g.:

host: {'class': 'mat-expansion-panel-set'},

So long as it's not a '[class]' binding (which will clobber other classes)

jelbourn · 2017-04-21T20:11:41Z

src/lib/expansion-panel/expansion-panel.ts

+export const EXPANSION_PANEL_ANIMATION_TIMING = '225ms cubic-bezier(0.4,0.0,0.2,1)';
+
+@Directive({
+  selector: 'md-expansion-panel-set, mat-expansion-panel-set',


A group of expansion panels is typically called an accordion. Accordions also have special a11y treatment on top of the standalone panel: https://www.w3.org/TR/wai-aria-practices-1.1/#accordion. It's possible that this maybe be a completely separate component that just shares some code with the expansion panel.

Maybe leave this out for now and add it in a follow-up?

jelbourn · 2017-04-21T20:15:12Z

src/lib/expansion-panel/expansion-panel.ts

+})
+export class MdExpansionPanelSet implements AfterContentInit, OnDestroy {
+  /** Whether the panel set should use flat styling. */
+  @Input('style') style: 'default' | 'flat' | 'wide' = 'default';


What are these different styles? I don't see anything about them in the spec. Is this something the user wouldn't be able to accomplish with css?

jelbourn · 2017-04-21T20:17:45Z

src/lib/expansion-panel/expansion-panel.ts

+  /** Whether the panel set should use flat styling. */
+  @Input('style') style: 'default' | 'flat' | 'wide' = 'default';
+  /** Whether the panel set should allow multiple open panels. */
+  @Input('multi') multi: boolean = false;


This should be a boolean property that uses coerceBooleanProperty

jelbourn · 2017-04-21T20:20:20Z

src/lib/expansion-panel/expansion-panel.ts

+  templateUrl: './expansion-panel.html',
+  encapsulation: ViewEncapsulation.None,
+  host: {
+    '[class.mat-expansion-panel]': 'true',


Same thing about 'class' here

jelbourn · 2017-04-21T20:21:06Z

src/lib/expansion-panel/expansion-panel.ts

+  /** Unique identifier for the expansion panel, to be used by aria attributes. */
+  private _expansionPanelId: number;
+  /** Event emitted for each expanded state change. */
+  @Output('expandChange') expandChange = new EventEmitter<boolean>();


Don't need the name when it's the same as the property

jelbourn · 2017-04-21T20:21:30Z

src/lib/expansion-panel/expansion-panel.ts

+  /** Event emitted for each expanded state change. */
+  @Output('expandChange') expandChange = new EventEmitter<boolean>();
+  /** Event emitted every time the expansion panel is closed. */
+  @Output('close') close = new EventEmitter<null>();


If there's no event object you should do EventEmitter<void>

arlowhite · 2017-05-25T18:51:54Z

src/lib/expansion/index.ts

@@ -0,0 +1,41 @@
+import {NgModule, ModuleWithProviders} from '@angular/core';
+import {CommonModule} from '@angular/common';
+import {BrowserAnimationsModule} from '@angular/platform-browser/animations';


Is this BrowserAnimationsModule import a problem?
I merged this PR into master and did a build, but my App now has this error:

Error: BrowserModule has already been loaded.

I modified publish-build-artifacts.sh to deploy to my repo and ran it.
See angular/material-builds@master...arlowhite:master
Load diff for material.js
+import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
You can see that this import has been added.

ng version

@angular/cli: 1.0.4
node: 6.9.1
os: linux x64
@angular/common: 4.1.3
@angular/material: 2.0.0-beta.5-25de2f2

Confirmed. Removing the import fixes the load error.
This build works: arlowhite/material2-builds@63ae5f5
Original does not: arlowhite/material2-builds@54a809f

EDIT: I confused myself earlier by moving my BrowserAnimationsModule import to my app's SharedModule instead of CoreModule, which causes the error in both builds.

arlowhite · 2017-05-25T20:11:45Z

The arrow icon is reversed from what the Material Guideline recommends:
https://material.io/guidelines/components/expansion-panels.html#expansion-panels-behavior
Arrow should point down when collapsed.

jelbourn

Last handful of comments from me

jelbourn · 2017-05-18T21:28:15Z

src/lib/expansion/expansion-panel.ts

+import {Subject} from 'rxjs/Subject';
+import 'rxjs/add/operator/takeUntil';
+import {CdkAccordion, CdkAccordionDisplayMode} from './accordion';
+import {CdkAccordionItem} from './accordion-item';


Lots of unused imports

jelbourn · 2017-05-18T21:28:28Z

src/lib/expansion/expansion-panel.ts

+  @Input() hideToggle: boolean = true;
+
+  /** The expansion panel style. */
+  @Input() panelStyle: string = 'default';


This appears to be unused now

jelbourn · 2017-05-18T21:29:25Z

src/lib/expansion/expansion-panel.ts

+    // Only emit events and update the internal value if the value changes.
+    if (this._expanded !== expanded) {
+      this._expanded = expanded;
+      if (expanded) {


(expanded ? this.opened : this.closed).emit();

jelbourn · 2017-05-18T21:30:10Z

src/lib/expansion/accordion.ts

+import {Subscription} from 'rxjs/Subscription';
+import {Subject} from 'rxjs/Subject';
+import 'rxjs/add/operator/takeUntil';
+import {CdkAccordionItem} from './accordion-item';


Lots of unused imports

jelbourn · 2017-05-18T21:31:11Z

src/lib/expansion/expansion-panel.ts

+  }
+
+  /** Toggles the expanded state of the panel. */
+  toggle(): void {


Should have public methods for open and close as well

jelbourn · 2017-06-02T22:50:44Z

src/lib/expansion/accordion-item.ts

+  }
+  private _expanded: boolean;
+
+  constructor(@Optional() @Host() public accordion: CdkAccordion,


Is @Host what you want here? Shouldn't you just get what you want from CdkAccordion by itself?

My thought was if you had a CdkAccordion inside of another CdkAccordion that you wanted to be sure you got the correct CdkAccordion, so we only look at the host component. However now that you point it out I see how flawed that logic is as it will get the first one it finds on its way up the tree.

jelbourn · 2017-06-02T22:53:12Z

src/lib/expansion/accordion.ts

+  set hideToggle(show: boolean) { this._hideToggle = coerceBooleanProperty(show); }
+  private  _hideToggle: boolean = false;
+
+  /** Whether the panel set should use flat styling. */


Comment should be updated (still written as a boolean). Should explain the difference between the modes.

jelbourn · 2017-06-02T22:54:28Z

src/lib/expansion/accordion.ts

+ * Directive whose purpose is to manage the expanded state of CdkAccordionItem children.
+ */
+@Directive({
+  selector: '[cdk-accordion]',


Why is this one an attribute instead of an element?

Also, we should make a separate <md-accordion> that is probably just

@Directive({ selector: 'md-accordion'}) export class MdAccordion extends CdkAccordion { }

Just so users don't have to mix md- and cdk-

The idea for it being an attribute rather than an element is because it is simply a containing item for the children who control there own spacing and styling. Since there is nothing in this directive that affects the document flow, it seemed to make sense that you could just mark another element as the containing element for the children.

jelbourn · 2017-06-02T23:00:08Z

src/lib/expansion/expansion-panel-header.ts

+    switch (event.keyCode) {
+      // Toggle for space and enter keys.
+      case SPACE:
+      case ENTER:


Probably want to preventDefault on these (space will scroll and enter may submit a form)

jelbourn · 2017-06-02T23:01:04Z

src/lib/expansion/expansion-panel.html

+  <div class="mat-expansion-panel-body">
+    <ng-content></ng-content>
+  </div>
+  <ng-content select="[mat-action-row], [md-action-row]"></ng-content>


These should use an element selector (e.g. <md-action-row>)

josephperrott · 2017-06-05T18:11:43Z

@jelbourn Comments addressed ready for a look

jelbourn

LGTM

jelbourn · 2017-06-05T21:42:42Z

@andrewseguin this one will need new build rules when syncing

otienoanyango · 2017-06-09T11:46:16Z

Really excited about this. Great job guys!

k-krupka · 2017-07-04T11:42:13Z

Even in beta, awesome.
When sb does something good, they should be told about this. Nice work then!

angular-automatic-lock-bot · 2019-09-06T15:01:20Z

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 Apr 21, 2017

josephperrott force-pushed the expansion-panel branch from 530b396 to e8e34bb Compare April 21, 2017 15:47

jelbourn changed the title ~~feat(MdExpansionPanel): Create Expansion Panel~~ feat(expansion-panel): initial version of expansion panel Apr 21, 2017

crisbeto reviewed Apr 21, 2017

View reviewed changes

crisbeto mentioned this pull request Apr 21, 2017

build: disallow uses of the "initial" value #4204

Merged

jelbourn reviewed Apr 21, 2017

View reviewed changes

josephperrott force-pushed the expansion-panel branch 6 times, most recently from 123a404 to 112f8f8 Compare April 29, 2017 00:36

jelbourn added the pr: needs rebase label May 1, 2017

josephperrott force-pushed the expansion-panel branch 5 times, most recently from aeff3d5 to 1638087 Compare May 2, 2017 16:33

josephperrott removed the pr: needs rebase label May 2, 2017

josephperrott force-pushed the expansion-panel branch 7 times, most recently from b658745 to 50ff89e Compare May 9, 2017 01:56

josephperrott force-pushed the expansion-panel branch 3 times, most recently from 7edcfcf to 3ff00ca Compare May 10, 2017 23:55

josephperrott force-pushed the expansion-panel branch 2 times, most recently from 23356a0 to 25de2f2 Compare May 24, 2017 03:57

arlowhite reviewed May 25, 2017

View reviewed changes

josephperrott force-pushed the expansion-panel branch 5 times, most recently from 6df441b to 34ce9c0 Compare May 31, 2017 23:43

csvn mentioned this pull request Jun 2, 2017

Proposal: md-expansion-panel #995

Closed

josephperrott force-pushed the expansion-panel branch 2 times, most recently from e89cecc to f8dbdc8 Compare June 2, 2017 20:50

jelbourn reviewed Jun 2, 2017

View reviewed changes

josephperrott force-pushed the expansion-panel branch 3 times, most recently from df68323 to e740a4e Compare June 3, 2017 18:01

jelbourn approved these changes Jun 5, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 5, 2017

josephperrott force-pushed the expansion-panel branch 2 times, most recently from ca99138 to 5ea821f Compare June 7, 2017 22:57

Create Expansion Panel.

66820fe

josephperrott force-pushed the expansion-panel branch from 5ea821f to 66820fe Compare June 7, 2017 23:00

andrewseguin merged commit cac7610 into angular:master Jun 7, 2017

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

		@@ -0,0 +1,6 @@
		<ng-content select="md-summary"></ng-content>

		@@ -0,0 +1,6 @@
		<ng-content select="md-summary"></ng-content>
		<div class="md-expansion-panel-body" [class.expanded]="expanded"

		@@ -0,0 +1,54 @@
		<h1>Single Expansion Panel</h1>
		<md-expansion-panel class="md-expansion-demo-width" #myPanel>

feat(expansion-panel): initial version of expansion panel #4191

feat(expansion-panel): initial version of expansion panel #4191

Conversation

josephperrott commented Apr 21, 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

jelbourn left a 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

arlowhite May 25, 2017 • edited Loading

Choose a reason for hiding this comment

arlowhite commented May 25, 2017

jelbourn left a 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

josephperrott commented Jun 5, 2017

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Jun 5, 2017

otienoanyango commented Jun 9, 2017

k-krupka commented Jul 4, 2017

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

arlowhite May 25, 2017 •

edited

Loading