Closed Bug 1416104 Opened 7 years ago Closed 6 years ago

React-ify animated property list

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 1 obsolete file)

Bug 1416104 - Part 1: Implement base.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 2: Make summary graph selectable.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 3: Implement header of animation detail pane.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 4: Implement base of animated property list.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 5: Implement animated property list header.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 6: Implement keyframes list.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 7: Make animated property list scrollable.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 8: Open detail pane when an animation was selected or number of displayed animation was one.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 9: Add close button.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 10: Add tests.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1416104 - Part 0: Combine action files and reducer files into one.
59 bytes, text/x-review-board-request
gl
: review+
Details 
In this bug, do React-ify keyframes list. 
It does not include keyframes graph.

* Basic layout for keyframes list.
* Implement progress tick for keyframes.
Blocks: 1416106
Severity: normal → enhancement
Priority: -- → P3
Hi Gabriel,
I'm sorry, may I rename KeyframesList and KeyframesItem to AnimatedPropertyList and AnimatedPropertyItem?
Because the item has not only keyframes but CSS property.
How do you think?

If ok, I'll change the bug title as well.
Flags: needinfo?(gl)
(In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> Hi Gabriel,
> I'm sorry, may I rename KeyframesList and KeyframesItem to
> AnimatedPropertyList and AnimatedPropertyItem?
> Because the item has not only keyframes but CSS property.
> How do you think?
> 
> If ok, I'll change the bug title as well.

Sounds fine.
Flags: needinfo?(gl)
Summary: React-ify keyframes list → React-ify animated property list
Comment on attachment 8934871 [details]
Bug 1416104 - Part 1: Implement base.

https://reviewboard.mozilla.org/r/205796/#review215644
Attachment #8934871 - Flags: review?(gl) → review+
Comment on attachment 8934872 [details]
Bug 1416104 - Part 2: Make summary graph selectable.

https://reviewboard.mozilla.org/r/205798/#review215646

::: devtools/client/inspector/animation/actions/selected-animation.js:13
(Diff revision 2)
> +
> +module.exports = {
> +  /**
> +   * Update selected animation.
> +   */
> +  updateSelectedAnimation(animation) {

All these new actions file and new reducers are telling me we should really just be bundling it all into the animation.js reducer state and then we can remove all but the animation.js reducer and action file

{
  animations: [],
  selectedAnimation: null,
  elementPickerEnable: false,
}

::: devtools/client/inspector/animation/components/AnimationItem.js:19
(Diff revision 2)
>  
>  class AnimationItem extends PureComponent {
>    static get propTypes() {
>      return {
>        animation: PropTypes.object.isRequired,
> +      animationSelected: PropTypes.object.isRequired,

s/animationSelected/selectedAnimation

::: devtools/client/inspector/animation/components/AnimationItem.js:42
(Diff revision 2)
> +    };
> +  }
> +
> +  componentWillReceiveProps(nextProps) {
> +    const { animation } = this.props;
> +    this.setState(

this.setState({

})
Attachment #8934872 - Flags: review?(gl) → review+
Comment on attachment 8934873 [details]
Bug 1416104 - Part 3: Implement header of animation detail pane.

https://reviewboard.mozilla.org/r/205800/#review215648

::: devtools/client/inspector/animation/components/graph/SummaryGraph.js:16
(Diff revision 2)
>  const AnimationName = createFactory(require("./AnimationName"));
>  const DelaySign = createFactory(require("./DelaySign"));
>  const EndDelaySign = createFactory(require("./EndDelaySign"));
>  const SummaryGraphPath = createFactory(require("./SummaryGraphPath"));
>  
> -const { getFormatStr, getStr, numberWithDecimals } = require("../../utils/l10n");
> +const { getFormattedTitle, getFormatStr, getStr, numberWithDecimals } = require("../../utils/l10n");

This is an eslint error.
Attachment #8934873 - Flags: review?(gl) → review+
Comment on attachment 8934874 [details]
Bug 1416104 - Part 4: Implement base of animated property list.

https://reviewboard.mozilla.org/r/205802/#review215650

::: devtools/client/inspector/animation/components/AnimationDetailContainer.js:39
(Diff revision 2)
>          }
>        )
>        :
> +      null,
> +      animation ?
> +      AnimatedPropertyListContainer()

We should indent everything below ?

This makes it a bit hard to read.
Attachment #8934874 - Flags: review?(gl) → review+
Comment on attachment 8934875 [details]
Bug 1416104 - Part 5: Implement animated property list header.

https://reviewboard.mozilla.org/r/205804/#review215652

::: devtools/client/inspector/animation/components/KeyframesProgressTickList.js:10
(Diff revision 2)
> +"use strict";
> +
> +const { createFactory, PureComponent } = require("devtools/client/shared/vendor/react");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +const { getFormatStr } = require("../utils/l10n");

This should appear in a new line after KeyframesProgressTickItem

::: devtools/client/themes/animation.css:12
(Diff revision 2)
>  :root {
>    --animation-even-background-color: rgba(0, 0, 0, 0.05);
>    --command-pick-image: url(chrome://devtools/skin/images/command-pick.svg);
>    --graph-right-offset: 10px;
>    --sidebar-width: 200px;
> +  --tick-line-style: 0.5px solid rgba(128, 136, 144, .5);

s/.5/0.5

::: devtools/client/themes/animation.css:12
(Diff revision 2)
>  :root {
>    --animation-even-background-color: rgba(0, 0, 0, 0.05);
>    --command-pick-image: url(chrome://devtools/skin/images/command-pick.svg);
>    --graph-right-offset: 10px;
>    --sidebar-width: 200px;
> +  --tick-line-style: 0.5px solid rgba(128, 136, 144, .5);

Where is this color coming from? I tried to searching for it in the codebase, but didn't find anything. I am a bit concern that a lot of colours here are not utilizing the photon colours and will probably all need to be updated before shipping.
Attachment #8934875 - Flags: review?(gl) → review+
Comment on attachment 8934876 [details]
Bug 1416104 - Part 6: Implement keyframes list.

https://reviewboard.mozilla.org/r/205806/#review215656

::: devtools/client/inspector/animation/components/AnimatedPropertyList.js:24
(Diff revision 2)
> +  }
> +
> +  constructor(props) {
> +    super(props);
> +
> +    this.state = {

this.state = {
  animatedPropertyMap: null
}

We should still fill in the state even if the property isn't assigned until the props are received.
Attachment #8934876 - Flags: review?(gl) → review+
Comment on attachment 8934877 [details]
Bug 1416104 - Part 7: Make animated property list scrollable.

https://reviewboard.mozilla.org/r/205808/#review215658
Attachment #8934877 - Flags: review?(gl) → review+
Comment on attachment 8934878 [details]
Bug 1416104 - Part 8: Open detail pane when an animation was selected or number of displayed animation was one.

https://reviewboard.mozilla.org/r/205810/#review215660

::: devtools/client/inspector/animation/components/App.js:52
(Diff revision 2)
>        selectAnimation,
>        setSelectedNode,
>        simulateAnimation,
>        toggleElementPicker,
>      } = this.props;
> +    const detailVisibility = isDetailVisible ? "animation-detail-visible" : "";

I think we can just inline this instead of having a constant.
Attachment #8934878 - Flags: review?(gl) → review+
Comment on attachment 8934879 [details]
Bug 1416104 - Part 9: Add close button.

https://reviewboard.mozilla.org/r/205812/#review215662

::: devtools/client/themes/animation.css:9
(Diff revision 2)
>  
>  /* Animation-inspector specific theme variables */
>  
>  :root {
>    --animation-even-background-color: rgba(0, 0, 0, 0.05);
> +  --close-button-image: url(chrome://devtools/skin/images/close.svg);

This probably doesn't need to be a variable since it is only used once.

This is probably true for --command-pick-image as well.
Attachment #8934879 - Flags: review?(gl) → review+
Comment on attachment 8934872 [details]
Bug 1416104 - Part 2: Make summary graph selectable.

https://reviewboard.mozilla.org/r/205798/#review215646

> All these new actions file and new reducers are telling me we should really just be bundling it all into the animation.js reducer state and then we can remove all but the animation.js reducer and action file
> 
> {
>   animations: [],
>   selectedAnimation: null,
>   elementPickerEnable: false,
> }

Okay, will do that.
Comment on attachment 8934872 [details]
Bug 1416104 - Part 2: Make summary graph selectable.

https://reviewboard.mozilla.org/r/205798/#review215646

> Okay, will do that.

Please let me do this change in patch 0.
Then, append the action and reducer which needs for this patch into there.
Comment on attachment 8934873 [details]
Bug 1416104 - Part 3: Implement header of animation detail pane.

https://reviewboard.mozilla.org/r/205800/#review215648

> This is an eslint error.

Sorry, I could not get the error with the command below in my environment.
./mach eslint devtools/client/inspector/animation/ 

I did the setup again anyway.
./mach eslint --setup
But, could not.

How are you checking the lint?
Comment on attachment 8934875 [details]
Bug 1416104 - Part 5: Implement animated property list header.

https://reviewboard.mozilla.org/r/205804/#review215652

> Where is this color coming from? I tried to searching for it in the codebase, but didn't find anything. I am a bit concern that a lot of colours here are not utilizing the photon colours and will probably all need to be updated before shipping.

It is same to tick line in netmonitor.
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/constants.js#296

And yes, I'll file a bug for photonize later.
Comment on attachment 8934879 [details]
Bug 1416104 - Part 9: Add close button.

https://reviewboard.mozilla.org/r/205812/#review215662

> This probably doesn't need to be a variable since it is only used once.
> 
> This is probably true for --command-pick-image as well.

For --command-pick-image, will do in another bug or patch.
Comment on attachment 8940625 [details]
Bug 1416104 - Part 0: Combine action files and reducer files into one.

https://reviewboard.mozilla.org/r/210852/#review216986
Attachment #8940625 - Flags: review?(gl) → review+
Status: NEW → ASSIGNED
Comment on attachment 8934880 [details]
Bug 1416104 - Part 10: Add tests.

https://reviewboard.mozilla.org/r/205814/#review219194

::: devtools/client/inspector/animation/test/browser_animation_AnimationDetail_close_button.js:17
(Diff revision 3)
> +  info("Checking close button in header of animation detail");
> +  await clickOnAnimation(animationInspector, panel, 0);
> +  const detailEl = panel.querySelector("#animation-container .controlled");
> +  const win = panel.ownerGlobal;
> +  isnot(win.getComputedStyle(detailEl).display, "none",
> +        "detailEl should be visibled before clicking close button");

NIT: There's a bit of uneven indenting all over these tests. I would just stick to 1 tab[2 spaces] when indenting in the new line.

::: devtools/client/inspector/animation/test/head.js:92
(Diff revision 3)
>  
>  /**
> + * Click on an animation in the timeline to select it.
> + *
> + * @param {AnimationInspector} animationInspector.
> + * @param {AnimationsPanel} panel The panel instance.

Put @param description on the next line
Attachment #8934880 - Flags: review?(gl) → review+
Attachment #8940625 - Attachment is obsolete: true
Ohhh, sorry, could you give r+ again becase I had removed patch 0.

Then, will land if the try has no problems.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94970608578a1180fb080ae82bb1b5191b646a77
Flags: needinfo?(gl)
Blocks: 1431573
Comment on attachment 8943554 [details]
Bug 1416104 - Part 0: Combine action files and reducer files into one.

https://reviewboard.mozilla.org/r/213902/#review219924

::: devtools/client/inspector/animation/components/NoAnimationPanel.js:42
(Diff revision 1)
>          L10N.getStr("panel.noAnimation")
>        ),
>        dom.button(
>          {
>            className: "animation-element-picker devtools-button"
> -                     + (elementPicker.isEnabled ? " checked" : ""),
> +                     + (elementPickerEnabled ? " checked" : ""),

Put the + in the line before.
Attachment #8943554 - Flags: review?(gl) → review+
Comment on attachment 8943554 [details]
Bug 1416104 - Part 0: Combine action files and reducer files into one.

https://reviewboard.mozilla.org/r/213902/#review219924

> Put the + in the line before.

I'll land after this fixing.
Thanks!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c897609672e6
Part 0: Combine action files and reducer files into one. r=gl
https://hg.mozilla.org/integration/autoland/rev/f9ef5865484a
Part 1: Implement base. r=gl
https://hg.mozilla.org/integration/autoland/rev/4c29e8818f8e
Part 2: Make summary graph selectable. r=gl
https://hg.mozilla.org/integration/autoland/rev/cd6a207773cf
Part 3: Implement header of animation detail pane. r=gl
https://hg.mozilla.org/integration/autoland/rev/31525f892e02
Part 4: Implement base of animated property list. r=gl
https://hg.mozilla.org/integration/autoland/rev/ad217aa1cc46
Part 5: Implement animated property list header. r=gl
https://hg.mozilla.org/integration/autoland/rev/37439dceeaaf
Part 6: Implement keyframes list. r=gl
https://hg.mozilla.org/integration/autoland/rev/ff74c42691e2
Part 7: Make animated property list scrollable. r=gl
https://hg.mozilla.org/integration/autoland/rev/1622c88dee9a
Part 8: Open detail pane when an animation was selected or number of displayed animation was one. r=gl
https://hg.mozilla.org/integration/autoland/rev/08b53d0a8765
Part 9: Add close button. r=gl
https://hg.mozilla.org/integration/autoland/rev/c33869ac6bc8
Part 10: Add tests. r=gl
Flags: needinfo?(gl)
Blocks: 1438072
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: