Closed Bug 1416106 Opened 7 years ago Closed 6 years ago

React-ify animated property item

Categories

(DevTools :: Inspector: Animations, enhancement, P3)

Product:
DevTools
Component:
Inspector: Animations
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

Bug 1416106 - Part 1: Implement base of property name.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 2: Implement compositor sign to property name.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 3: Implement compositor warning.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 4: Add test for property name component.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 5: Implement base of keyframes graph.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 6: Implement distance graph.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 7: Implement color graph.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 8: Implement discrete graph.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 9: Set styles to graph.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 10: Implement easing hit.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 11: Implement keyframe markers.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 12: Add tests.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416106 - Part 13: Remove extra margin-bottom.
59 bytes, text/x-review-board-request
gl
: review+
Details 
In this bug, implement following features.

* graph
* keyframe markers
* keyframe easing path
Severity: normal → enhancement
Priority: -- → P3
Summary: React-ify keyframes item → React-ify animated property item
Comment on attachment 8943834 [details]
Bug 1416106 - Part 1: Implement base of property name.

https://reviewboard.mozilla.org/r/213932/#review219916

::: devtools/client/inspector/animation/components/AnimatedPropertyName.js:26
(Diff revision 1)
> +    return dom.div(
> +      {
> +        className: "animated-property-name",
> +      },
> +      dom.span(
> +        {

Either null or {} in one line

::: devtools/client/inspector/animation/components/AnimatedPropertyName.js:26
(Diff revision 2)
> +    return dom.div(
> +      {
> +        className: "animated-property-name",
> +      },
> +      dom.span(
> +        {

{} in one line
Attachment #8943834 - Flags: review?(gl) → review+
Comment on attachment 8943835 [details]
Bug 1416106 - Part 2: Implement compositor sign to property name.

https://reviewboard.mozilla.org/r/213934/#review221934

::: devtools/client/inspector/animation/components/AnimatedPropertyName.js:15
(Diff revision 2)
>  
>  class AnimatedPropertyName extends PureComponent {
>    static get propTypes() {
>      return {
>        property: PropTypes.string.isRequired,
> +      state: PropTypes.object.isRequired,

state can be null judging from looking at getPropertyState.

Having isRequired would provide a warning if state is null in this case. I would do either 
PropTypes.oneOfType([null, PropTypes.object]).isRequired to preserve the type checking or remove the isRequired.
Attachment #8943835 - Flags: review?(gl) → review+
Comment on attachment 8943836 [details]
Bug 1416106 - Part 3: Implement compositor warning.

https://reviewboard.mozilla.org/r/213936/#review221936
Attachment #8943836 - Flags: review?(gl) → review+
Comment on attachment 8943838 [details]
Bug 1416106 - Part 5: Implement base of keyframes graph.

https://reviewboard.mozilla.org/r/213940/#review221938
Attachment #8943838 - Flags: review?(gl) → review+
Comment on attachment 8943837 [details]
Bug 1416106 - Part 4: Add test for property name component.

https://reviewboard.mozilla.org/r/213938/#review225902

::: devtools/client/inspector/animation/test/browser_animation_animated-property-name.js:6
(Diff revision 3)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test following animated property name component features.

Test the following animated property name component features:

::: devtools/client/inspector/animation/test/browser_animation_animated-property-name.js:11
(Diff revision 3)
> +// Test following animated property name component features.
> +// * name of property
> +// * display compositor sign when the property was running on compositor.
> +// * display warning when the property is runnable on compositor but was not.
> +
> +const TEST_CASES = [

We typically call this TEST_DATA.
s/TEST_CASES/TEST_DATA

::: devtools/client/inspector/animation/test/browser_animation_animated-property-name.js:37
(Diff revision 3)
> +  const animatedPropertyNameEls = panel.querySelectorAll(".animated-property-name");
> +  is(animatedPropertyNameEls.length, TEST_CASES.length,
> +    `Number of animated property name elements should be ${ TEST_CASES.length }`);
> +
> +  for (const [index, animatedPropertyNameEl] of animatedPropertyNameEls.entries()) {
> +    const testCase = TEST_CASES[index];

s/testCase/testData
Attachment #8943837 - Flags: review?(gl) → review+
Comment on attachment 8943839 [details]
Bug 1416106 - Part 6: Implement distance graph.

https://reviewboard.mozilla.org/r/213942/#review225916

::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:19
(Diff revision 3)
> +  getPreferredProgressThresholdByKeyframes,
> +  toPathString,
> +} = require("../../utils/graph-helper");
> +
> +/*
> + * This class is abstract for computed style path of keyframes.

This class is an abstraction

::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:20
(Diff revision 3)
> +  toPathString,
> +} = require("../../utils/graph-helper");
> +
> +/*
> + * This class is abstract for computed style path of keyframes.
> + * Subclass of this should implement following methods at least.

should implement the following methods:

::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:52
(Diff revision 3)
> +      values: PropTypes.array.isRequired,
> +    };
> +  }
> +
> +  /**
> +   * Create path segment for between given startValue and endValue.

Return an array containing the path segments between the given start and end keyframe values.

::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:62
(Diff revision 3)
> +   *        Ending keyframe.
> +   * @return {Array}
> +   *         Array of path segment.
> +   *         [{x: {Number} time, y: {Number} segment value}, ...]
> +   */
> +  createPathSegments(startValue, endValue) {

I got confused by createPathSegments that is import and this function name. Perhaps it is best we rename this to getPathSegments.

::: devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraph.js:29
(Diff revision 3)
> +      values,
> +    } = this.props;
> +
>      return dom.div(
>        {
>          className: "keyframes-graph"

Add a comma at the end.

::: devtools/client/inspector/animation/utils/graph-helper.js:8
(Diff revision 3)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  // BOUND_EXCLUDING_TIME should be less than 1ms and is used to exclude start
>  // and end bounds when dividing  duration in createPathSegments.

NIT: There is 2 spaces after dividing. Remove 1 of the spaces.

::: devtools/client/inspector/animation/utils/graph-helper.js:212
(Diff revision 3)
> +}
> +
> +/**
> + * Return preferred progress threshold by keyframes.
> + *
> + * @param {Array} keyframes - keyframes.

Put the @param description on the next line like we have done in this file.
Attachment #8943839 - Flags: review?(gl) → review+
Comment on attachment 8943840 [details]
Bug 1416106 - Part 7: Implement color graph.

https://reviewboard.mozilla.org/r/213944/#review225922

::: devtools/client/inspector/animation/components/keyframes-graph/ColorPath.js:35
(Diff revision 3)
> +
> +  getPropertyValue(keyframe) {
> +    return keyframe.value;
> +  }
> +
> +  setup({ values }) {

Rename this updateState.

::: devtools/client/inspector/animation/components/keyframes-graph/ColorPath.js:54
(Diff revision 3)
> +        maxObject.value1 = value1;
> +        maxObject.value2 = value2;
> +      }
> +    }
> +
> +    this.maxDistance = maxObject.distance;

Can we not store this as a state with this.state?

::: devtools/client/inspector/animation/components/keyframes-graph/ColorPath.js:55
(Diff revision 3)
> +        maxObject.value2 = value2;
> +      }
> +    }
> +
> +    this.maxDistance = maxObject.distance;
> +    this.baseValue =

Same as above.

::: devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraphPath.js:41
(Diff revision 3)
>  
>    componentDidMount() {
>      this.updateState();
>    }
>  
> +  getPathClass(type) {

Rename this to be getPathComponent. I thought this was a CSS class name otherwise.
Attachment #8943840 - Flags: review?(gl) → review+
Comment on attachment 8943841 [details]
Bug 1416106 - Part 8: Implement discrete graph.

https://reviewboard.mozilla.org/r/213946/#review225924

::: devtools/client/inspector/animation/animation.js:164
(Diff revision 3)
>  
>      return animatedPropertyMap;
>    }
>  
> +  /**
> +   * Set styles to an element then return computed style of specifed property.

Return the computed style of the specified property after setting the given styles to the simulated element.

::: devtools/client/inspector/animation/components/keyframes-graph/DiscretePath.js:22
(Diff revision 3)
> +  }
> +
> +  constructor(props) {
> +    super(props);
> +
> +    this.setup(props);

Rename this to updateState

::: devtools/client/inspector/animation/components/keyframes-graph/DiscretePath.js:48
(Diff revision 3)
> +      if (!discreteValues.includes(style)) {
> +        discreteValues.push(style);
> +      }
> +    }
> +
> +    this.discreteValues = discreteValues;

Can we store this in this.state?
Attachment #8943841 - Flags: review?(gl) → review+
Comment on attachment 8943842 [details]
Bug 1416106 - Part 9: Set styles to graph.

https://reviewboard.mozilla.org/r/213948/#review225926
Attachment #8943842 - Flags: review?(gl) → review+
Comment on attachment 8943843 [details]
Bug 1416106 - Part 10: Implement easing hit.

https://reviewboard.mozilla.org/r/213950/#review225928

::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:156
(Diff revision 3)
> +        },
> +        dom.title({}, startKeyframe.easing),
> +        dom.path(
> +          {
> +            d: `M${ hintSegments[0].x },${ hintSegments[0].y } `
> +               + toPathString(hintSegments),

Move the + to previous line

::: devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraphPath.js:91
(Diff revision 3)
>      return dom.svg(
>        {
>          className: "keyframes-graph-path",
>          preserveAspectRatio: "none",
> -        viewBox: `0 -${ DEFAULT_GRAPH_HEIGHT } `
> -                 + `${ DEFAULT_KEYFRAMES_GRAPH_DURATION } ${ DEFAULT_GRAPH_HEIGHT }`,
> +        viewBox: `0 -${ DEFAULT_GRAPH_HEIGHT + strokeWidthInViewBox } `
> +                 + `${ DEFAULT_KEYFRAMES_GRAPH_DURATION } `

Move the + to the end of the previous line

::: devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraphPath.js:92
(Diff revision 3)
>        {
>          className: "keyframes-graph-path",
>          preserveAspectRatio: "none",
> -        viewBox: `0 -${ DEFAULT_GRAPH_HEIGHT } `
> -                 + `${ DEFAULT_KEYFRAMES_GRAPH_DURATION } ${ DEFAULT_GRAPH_HEIGHT }`,
> +        viewBox: `0 -${ DEFAULT_GRAPH_HEIGHT + strokeWidthInViewBox } `
> +                 + `${ DEFAULT_KEYFRAMES_GRAPH_DURATION } `
> +                 + `${ DEFAULT_GRAPH_HEIGHT + strokeWidthInViewBox * 2 }`,

Same as above.

::: devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraphPath.js:97
(Diff revision 3)
> +                 + `${ DEFAULT_GRAPH_HEIGHT + strokeWidthInViewBox * 2 }`,
>        },
>        path(
>          {
>            componentWidth,
> +          easingHintStrokeWidth: DEFAULT_EASING_HINT_STROKE_WIDTH,

Since we are passing in a constant that is required here, let's not pass this as a prop and just require the DEFAULT_EASING_HINT_STROKE_WIDTH in those components instead.
Attachment #8943843 - Flags: review?(gl) → review+
Comment on attachment 8943844 [details]
Bug 1416106 - Part 11: Implement keyframe markers.

https://reviewboard.mozilla.org/r/213952/#review225930
Attachment #8943844 - Flags: review?(gl) → review+
Comment on attachment 8943843 [details]
Bug 1416106 - Part 10: Implement easing hit.

https://reviewboard.mozilla.org/r/213950/#review225928

> Since we are passing in a constant that is required here, let's not pass this as a prop and just require the DEFAULT_EASING_HINT_STROKE_WIDTH in those components instead.

Child components should use this stroke width since we expand the viewBox of SVG by this value. (I want to avoid that we specify in each components)
Also, most of default value are passing from here. Aspect from the consistency as well, I think it is better to pass this value from here. 
How do you think?
Comment on attachment 8943840 [details]
Bug 1416106 - Part 7: Implement color graph.

https://reviewboard.mozilla.org/r/213944/#review225922

> Can we not store this as a state with this.state?

Although we can store to the state, according to the document[1], we should not call setState from constructor. However, we can set initial state directly in the constructor. I'll try instead.

[1] https://reactjs.org/docs/react-component.html#constructor
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ac3e80d1402a12640e753fd4e2ee3fbb8ef2f1f

Also, I added one more patch for css to remove extra margin of the list.
Comment on attachment 8951110 [details]
Bug 1416106 - Part 13: Remove extra margin-bottom.

https://reviewboard.mozilla.org/r/220364/#review226330
Attachment #8951110 - Flags: review?(gl) → review+
Comment on attachment 8943843 [details]
Bug 1416106 - Part 10: Implement easing hit.

https://reviewboard.mozilla.org/r/213950/#review225928

> Child components should use this stroke width since we expand the viewBox of SVG by this value. (I want to avoid that we specify in each components)
> Also, most of default value are passing from here. Aspect from the consistency as well, I think it is better to pass this value from here. 
> How do you think?

Oh I see. Will drop this issue.
Comment on attachment 8943845 [details]
Bug 1416106 - Part 12: Add tests.

https://reviewboard.mozilla.org/r/213954/#review226338

::: devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path.js:6
(Diff revision 5)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test for following ComputedValuePath component works.

Test for the following ComputedValuepath component:

::: devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path.js:7
(Diff revision 5)
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test for following ComputedValuePath component works.
> +// * element existance

s/existance/existence/g

::: devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path.js:12
(Diff revision 5)
> +// * element existance
> +// * path segments
> +// * fill color by animation type
> +// * stop color if the animation type is color
> +
> +const TEST_CASES = [

s/TEST_CASES/TEST_DATA

::: devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js:11
(Diff revision 5)
> +// Test for following easing hint in ComputedValuePath.
> +// * element existance
> +// * path segments
> +// * hint text
> +
> +const TEST_CASES = [

s/TEST_CASES/TEST_DATA

::: devtools/client/inspector/animation/test/browser_animation_keyframes-graph_computed-value-path_easing-hint.js:197
(Diff revision 5)
> +add_task(async function () {
> +  await addTab(URL_ROOT + "doc_multi_easings.html");
> +
> +  const { inspector, panel } = await openAnimationInspector();
> +
> +  for (const testCase of TEST_CASES) {

s/testCase/testData/g

::: devtools/client/inspector/animation/test/head.js:212
(Diff revision 5)
>      await animationInspector.once("animation-summary-graph-rendered");
>    }
>  };
>  
>  /**
> + * Check the <stop> element in given linearGradient.

Check the <stop> element in the given linearGradientEl for the correct offset and color attributes.

::: devtools/client/inspector/animation/test/head.js:316
(Diff revision 5)
>  
>    return null;
>  }
> +
> +/**
> + * Find <stop> element which has given offset from given <svg> element.

Find the <stop> element which has the given offset in the given linearGradientEl.
Attachment #8943845 - Flags: review?(gl) → review+
Thank you very much, Gabriel!
Will land if the try has no problems.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb2ae2a86debb8fe71145dd3386ee46bf0cdecf
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5371c9a632fc
Part 1: Implement base of property name. r=gl
https://hg.mozilla.org/integration/autoland/rev/1a0183c9ee73
Part 2: Implement compositor sign to property name. r=gl
https://hg.mozilla.org/integration/autoland/rev/8e8ce2089341
Part 3: Implement compositor warning. r=gl
https://hg.mozilla.org/integration/autoland/rev/3801cbab320d
Part 4: Add test for property name component. r=gl
https://hg.mozilla.org/integration/autoland/rev/3f449e2f4ef1
Part 5: Implement base of keyframes graph. r=gl
https://hg.mozilla.org/integration/autoland/rev/729b7060c1e7
Part 6: Implement distance graph. r=gl
https://hg.mozilla.org/integration/autoland/rev/b1365cf5d62b
Part 7: Implement color graph. r=gl
https://hg.mozilla.org/integration/autoland/rev/0070a351a4a4
Part 8: Implement discrete graph. r=gl
https://hg.mozilla.org/integration/autoland/rev/5df3265bb609
Part 9: Set styles to graph. r=gl
https://hg.mozilla.org/integration/autoland/rev/f20d1625e15b
Part 10: Implement easing hit. r=gl
https://hg.mozilla.org/integration/autoland/rev/d717511990c4
Part 11: Implement keyframe markers. r=gl
https://hg.mozilla.org/integration/autoland/rev/481dab8f3670
Part 12: Add tests. r=gl
https://hg.mozilla.org/integration/autoland/rev/d9543f62a0f3
Part 13: Remove extra margin-bottom. r=gl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: