feat(shape): Adding MDCShape, an experimental subsystem #1448

lynnmercier · 2017-10-18T19:36:41Z

Also updated our webpack config and dependency test to allow demo-only JS.

Garbee · 2017-10-18T21:04:01Z

demos/shape/heart/foundation.js

+    const flatHeight = this.generateBaseFlatHeight_(height, padding);
+    const curveHeight = this.generateBaseHeight_(height, padding) - flatHeight;
+    const realWidth = this.generateBaseWidth_(width, padding) - flatWidth;
+    return 'c 0,' + (curveHeight - this.generateBaseCurveHeight2_(height, padding))


This looks like a good place to use template strings in JS to make this return statement a bit simpler.

Garbee · 2017-10-18T21:04:17Z

demos/shape/heart/foundation.js

+      + (-1 * (realWidth - this.generateBaseCurveWidth_(width, padding)))
+      + ',' + (curveHeight - this.generateBaseCurveHeight_(height, padding))
+      + (-1 * realWidth) + ',' + curveHeight
+      +'l ' + (-1 * flatWidth) + ',' + flatHeight;


nit: space after first + operator.

Another nit... do we not prefer trailing operators rather than leading? (Maybe moot if we reorganize this and use a template string.)

My Outlined Text Field prototype uses leading operators, and the king of SVG on the web Mike Bostock always uses leading operators. I am open to more clearly defining our convention here, and am not married to leading operators.

I guess it comes down to whether we want to adhere to Google's JS style guide for this, which promotes breaking after the operator, not before.

If we want to enforce this, we can use https://eslint.org/docs/rules/operator-linebreak

I think the closer we are to the Google style guide the better. Since that is the companies encouraged policy on coding style.

However, I still think shifting these to template literals would net a simpler codebase to maintain long-term.

kfranqueiro · 2017-10-19T13:16:09Z

packages/mdc-shape/_variables.scss

+//
+
+$mdc-shape-shadow-padding: 56px;
+$mdc-shape-shadow-negative-padding: -1 * $mdc-shape-shadow-padding;


Is there a motive behind making this a variable rather than simply doing -1 * mdc-shape-shadow-padding in the 2 cases where this is needed? The presence of a separate variable makes me think there'd be more to it than simply negating the other one.

uh nope, this was silly. I removed it.

kfranqueiro · 2017-10-19T13:20:20Z

packages/mdc-shape/foundation.js

+  }
+}
+
+const UMBRA_OFFSET_Y = [


Why are these constants here rather than in a constants file? Also, are these more directly related to elevation rather than shapes? I suspect these correspond to styles in mdc-elevation/_variables.scss and it would be really easy for these to get out of sync with those, being in a completely separate package.

Is there any other stuff here that should be considered for mdc-elevation instead? Should shapes be dependent on elevation?

Moved it to mdc-elevation!

kfranqueiro · 2017-10-19T13:22:56Z

packages/mdc-shape/component.js

+
+  initialSyncWithDOM() {
+    this.foundation_.setBackground(getBackgroundFromDOM(this.root_), false);
+    this.foundation_.setElevation(getElevationFromDOM(this.root_), false);


AFAICT, getElevationFromDOM assumes that CSS custom properties work, which in turn makes me suspect we need to be feature-detecting this the same way that we're doing in ripple (and IIRC a couple of other places, that probably need updating)...

Maybe it's time for #1104?

What's our plan for browsers that don't support custom CSS properties?

OTOH, is there a reason that setting elevation needs to be done this way rather than actually reusing mdc-elevation classes (esp. if there's any desire to support no-JS shapes), or just using a data attribute if this is solely for the purpose of initialization via JS?

Please... With the initialisms...

Theres no support for no-JS shapes...Shape JS has to draw the shape on a canvas, and it has to draw all the shadows on the canvas. The existing Sass mixins for elevation won't work with shapes because the shadows won't properly follow the irregular edges of a shape.

kfranqueiro · 2017-10-19T13:23:41Z

packages/mdc-shape/foundation.js

+    this.elevating_ = false;
+    this.elevation_ = value;
+
+    if (!animate) {


Suggestion: flip the if/else so that the if performs a positive comparison?

kfranqueiro · 2017-10-19T13:27:06Z

scripts/check-pkg-for-release.js


 main();

 function main() {
  checkPublicConfigForNewComponent();
  if (pkg.name !== MASTER_PKG.name) {
    checkNameIsPresentInAllowedScope();
+  }
+  if (NOT_PUBLISHABLE.indexOf(pkg.name) !== -1) {


Uh... is this inverted? IIUC this will only execute the statements below if the package is not publishable.

oops, fixed this up

kfranqueiro · 2017-10-19T13:38:54Z

demos/shape/shape.scss

+}
+.mdc-shape {
+  --mdc-shape-elevation: 4;
+  --mdc-shape-background: #FF00FF;


Nit (here and anywhere else applicable): lowercase hex values

kfranqueiro · 2017-10-19T13:40:01Z

packages/mdc-shape/README.md

+
+```html
+<div class="mdc-shape">
+  <canvas class="mdc-shape__canvas"></canvas>


Why is this a canvas and does it always have to be? Is shape expected to eventually work on other components?

It does always have to be a canvas.

Initially we had an implementation that clipped divs with clip-path, then applied a drop-shadow filter. This has a couple performance problems

Calculating drop-shadow requires iterating over every pixel within that div. So the larger the div the slower the performance

The drop-shadow is repainted (and therefore recalculated) whenever the content underneath the clipped div changed. (Technically, whenever any content in the same GPU tile changes.) This means if you were scrolling content under the clipped div, the scroll could become janky.

Also for working with other components...yes some day. Since this is still experimental we've only built some offhand prototypes (like for button). In the future most components will probably have some "shape" version, in addition to the default version, which has a modified HTML structure to include canvas. Obviously, these will be very heavy weight components and should be used with discretion.

kfranqueiro · 2017-10-19T13:42:59Z

packages/mdc-shape/mdc-shape.scss

+
+@import "./variables";
+
+$mdc-shape-shadow-double-padding: 2 * $mdc-shape-shadow-padding;


I'm confused why we're doing this here yet we have mdc-shape-shadow-negative-padding in variables.scss. For that matter, maybe it'd be easier to understand if we simply set all of top/left/bottom/right to the same thing, instead of requiring this math?

I'm also not sure I understand what the padding on each side of the canvas is useful for?

I think I was struggling with how to use a Sass variable within a calc function....fixed it now

The padding is to make sure the canvas is large enough to cover the highest elevation (24) with the largest blur (56px). This way there will always be enough room on the canvas to draw the shadow, regardless of what elevation a shape is at.

kfranqueiro · 2017-10-19T13:45:00Z

packages/mdc-shape/util.js

+    elevation = parseInt(mdcShapeElevation);
+  }
+
+  return elevation;


This is going to silently return 0 in the case where elevation can't be computed by these means. Is that what we want?

kfranqueiro · 2017-10-19T13:47:01Z

packages/mdc-shape/README.md

+mdcShape.elevation = 4;
+```
+
+The second way is to use custom CSS properties.


Should we really expose 2 ways to do the same thing, especially if the 2nd way won't work in all browsers? (I have more thoughts on the CSS custom properties approach in another comment)

Fair enough, I removed the custom CSS properties approach.

amsheehan · 2017-10-19T14:42:58Z

demos/shape/heart/foundation.js

+    +this.generateLeftBase_(width, height, padding)
+    +this.generateTopLeftCurve_(width, height, padding)
+    +this.generateTopRightCurve_(width, height, padding)
+    +this.generateRightBase_(width, height, padding);


If we are adding a space after the operator elsewhere, we should do the same here.

amsheehan · 2017-10-19T14:46:07Z

demos/shape/heart/foundation.js

+    +this.generateRightBase_(width, height, padding);
+  }
+  generateBaseWidth_(width, padding) {
+    return ((width - (padding * 2)) / 2);


parens around padding * 2 is unnecessary.

amsheehan · 2017-10-19T14:46:52Z

demos/shape/heart/foundation.js

+    return this.generateBaseWidth_(width, padding) * 0.52;
+  }
+  generateBaseHeight_(height, padding) {
+    return (height - (padding * 2)) * 0.67;


Same comment, remove parens around padding * 2

amsheehan · 2017-10-19T14:49:03Z

demos/shape/heart/foundation.js

+      +'l ' + (-1 * flatWidth) + ',' + flatHeight;
+  }
+  generateOutsideWidth_(width, padding) {
+    return ((width - (padding * 2)) / 2) * 0.55;


Remove parens around padding * 2

amsheehan · 2017-10-19T14:49:17Z

demos/shape/heart/foundation.js

+    return this.generateOutsideWidth_(width, padding) * 0.44;
+  }
+  generateInsideWidth_(width, padding) {
+    return ((width - (padding * 2)) / 2) * 0.45;


Remove parens around padding * 2

amsheehan · 2017-10-19T17:53:37Z

packages/mdc-shape/foundation.js

+
+  generateClipPath_() {
+    return this.generatePath_(
+      this.adapter_.getCanvasWidth() - (numbers.SHADOW_PADDING * 2),


SHADOW_PADDING is so metal. 🤘

amsheehan · 2017-10-19T18:01:12Z

packages/mdc-shape/foundation.js

+      finished = true;
+    }
+
+    this.redraw();


Seems like stuff is happening in init() that doesn't need to happen every time redraw() is called. Doesn't seem destructive yet, just seems unnecessary.

I moved some of the canvas stuff to a separate method. This way we don't re-create2dRenderingContext on every call to redraw.

The sync with canvas is important though, because the canvas could have changed size.

Under what circumstances do we expect redraw to be called? We have a layout method in a lot of components intended to handle resizes, should we do the sync with canvas there instead, or is this method effectively serving a similar purpose to layout in other components?

amsheehan · 2017-10-19T18:02:05Z

packages/mdc-shape/foundation.js

+    this.redraw();
+
+    if (!finished && this.elevating_) {
+      this.animationFrameId_ = requestAnimationFrame(() => this.elevate_());


Is this going to be hard to test?

I wrote a test!

amsheehan · 2017-10-19T18:03:47Z

packages/mdc-shape/foundation.js

+  }
+
+  clear_() {
+    this.ctx_.clearRect(0, 0, this.adapter_.getCanvasWidth(), this.adapter_.getCanvasHeight());


I think we should rethink redraw() :)

amsheehan · 2017-10-19T18:04:54Z

packages/mdc-shape/foundation.js

+  }
+}
+
+const UMBRA_OFFSET_Y = [


lynnmercier · 2017-11-02T21:47:46Z

Resolves #1535

…cript

kfranqueiro · 2017-11-17T21:37:57Z

packages/mdc-shape/README.md

+```
+
+We recommend you put any content for the shape inside the "clipped" element *after* the `mdc-shape__canvas` element. The "clipped" element must contain the style `clip-path:url(#FOO_ID)`, where `FOO_ID` corresponds to the value you assign to the `clipPath` element's `id` attribute. Using the same ID between the "clipped" element and the `clipPath` element effectively clips any content to the shape. It is important `mdc-shape__canvas` is before the "clipped" element, otherwise the clipping effect will clip the shadows drawn by `MDCShapeFoundation`.
+


Clipped seems like an appropriate word to me, but maybe introduce the vocabulary by outlining the purpose that each of the 3 elements serves in a bulleted list, then we don't have to air-quote "clipped" everywhere?

kfranqueiro · 2017-11-17T21:41:58Z

packages/mdc-shape/README.md

+
+Returns an object which has the shape of a CanvasRenderingContext2d instance. An easy way to achieve this is simply `this.root_.querySelector(mdc.shape.MDCShapeFoundation.SHAPE_SELECTOR).getContext('2d');`
+
+The full API must cover:


Nit: "cover" -> "include"? Might sound a bit more natural.

kfranqueiro · 2017-11-17T23:11:35Z

packages/mdc-shape/README.md

+
+### Shape Customization
+
+To customize your shape's elevation and background color, use the MDCShape's API.


Nit: "use the MDCShape's API" sounds a bit awkward... "use MDCShape's API"? "use the MDCShape API"? "use the methods on the MDCShape instance"?

kfranqueiro · 2017-11-17T23:12:00Z

packages/mdc-shape/README.md

+To modify the elevation of a shape, set the background
+
+```
+mdcShape.background = '#FOO';


Nit: lowercase f in the color hex value

kfranqueiro · 2017-11-17T23:26:13Z

packages/mdc-shape/component.js

+  }
+
+  createAdapter() {
+    return {setCanvasWidth: (value) => this.canvas_.width = value,


Nit: newline after {? (Not sure if there's a way to get lint to catch this without complaining about e.g. single-line cases)

kfranqueiro · 2017-11-17T23:42:07Z

packages/mdc-shape/foundation.js

+
+  drawAmbientShadow_(devicePixelRatio, noSpreadPath) {
+    this.ctx_.fillStyle = '#FFF';
+    this.ctx_.shadowColor = 'rgba(0, 0, 0, 0.12)';


Should these alpha values really be hard-coded here? Do they correspond to elevation constants we've added?

For that matter, is shape shadow color supposed to be parameterizable?

kfranqueiro · 2017-11-17T23:44:01Z

packages/mdc-shape/foundation.js

+ * limitations under the License.
+ */
+import {MDCFoundation} from '@material/base';
+import {constants} from '@material/elevation';


I might suggest making this have a more specific name like elevationConstants since this package also has a constants module (though we're importing specific members from that). WDYT?

kfranqueiro · 2017-11-18T00:08:00Z

test/unit/mdc-shape/foundation.test.js

+  const renderingContext = MDCShapeFoundation.defaultAdapter.create2dRenderingContext();
+  const renderingContextMethods =
+    Object.keys(renderingContext).filter((k) => typeof renderingContext[k] === 'function');
+  assert.deepEqual(renderingContextMethods.slice().sort(), ['scale', 'clearRect', 'fill'].slice().sort());


Is either slice call here required?

kfranqueiro · 2017-11-18T00:10:49Z

test/unit/mdc-shape/foundation.test.js

+
+  assert.equal(fillCount, 4);
+  assert.equal(shadowBlurs[0], 1);
+  assert.equal(shadowBlurs[1], 0.4);


Should some of these values (either here or below after flushing) be looked up from the constants in mdc-elevation?

kfranqueiro · 2017-11-18T00:12:09Z

test/unit/mdc-shape/foundation.test.js

+  td.verify(mockAdapter.createPath2D('m 56.2 56.2 h 100 v 200'));
+
+  // requires 9 frames to finish animation
+  for (let i=0; i<9; i++) {


I'm a bit confused why this is necessary... isn't our mock raf implementation supposed to flush all pending animation frames at once?

(IE11 and Edge lack full Path2D support though)

feat(shape): Adding MDCShape, an experimental subsystem

4b779de

Also updated our webpack config and dependency test to allow demo-only JS.

kfranqueiro self-requested a review October 18, 2017 19:38

Garbee reviewed Oct 18, 2017

View reviewed changes

kfranqueiro suggested changes Oct 19, 2017

View reviewed changes

amsheehan suggested changes Oct 19, 2017

View reviewed changes

lynnmercier added 4 commits November 3, 2017 14:20

Merge branch 'master' into feat/shape

c0d9343

first pass at fixing format

c1ae11d

more fixes, removed css custom properties, and fixed the dependency s…

1672f05

…cript

Merge branch 'master' into feat/shape

7591a3f

kfranqueiro suggested changes Nov 18, 2017

View reviewed changes

amsheehan assigned kfranqueiro Dec 11, 2017

WIP Fix error preventing testing in IE11

83c022c

(IE11 and Edge lack full Path2D support though)

lynnmercier closed this Apr 11, 2018

lynnmercier deleted the feat/shape branch May 31, 2018 22:48


		@import "./variables";

		$mdc-shape-shadow-double-padding: 2 * $mdc-shape-shadow-padding;

		```

		We recommend you put any content for the shape inside the "clipped" element after the `mdc-shape__canvas` element. The "clipped" element must contain the style `clip-path:url(#FOO_ID)`, where `FOO_ID` corresponds to the value you assign to the `clipPath` element's `id` attribute. Using the same ID between the "clipped" element and the `clipPath` element effectively clips any content to the shape. It is important `mdc-shape__canvas` is before the "clipped" element, otherwise the clipping effect will clip the shadows drawn by `MDCShapeFoundation`.


		Returns an object which has the shape of a CanvasRenderingContext2d instance. An easy way to achieve this is simply `this.root_.querySelector(mdc.shape.MDCShapeFoundation.SHAPE_SELECTOR).getContext('2d');`

		The full API must cover:


		### Shape Customization

		To customize your shape's elevation and background color, use the MDCShape's API.

feat(shape): Adding MDCShape, an experimental subsystem #1448

feat(shape): Adding MDCShape, an experimental subsystem #1448

Conversation

lynnmercier commented Oct 18, 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

kfranqueiro Oct 19, 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

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

lynnmercier commented Nov 2, 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

kfranqueiro Oct 19, 2017 •

edited

Loading