1334225 - autoland-ui: Try support

Status: RESOLVED FIXED

(Conduit :: General, defect)

Description

[Mark Côté [:mcote]]

Attachment: MozReview Try dialog

In addition to the main autoland function (landing to the mainline repo, e.g. the hg.mozilla.org/integration/autoland), we need to support landing to Try.  This requires a few extra features, namely, a place in the UI to specify a Try string, and integrating releng's Try chooser, which is a graphical way to build up a Try string.

See attached screenshot for what this dialog looks like now, in MozReview.

Comment 1

[Israel Madueme [:imadueme]]

Attachment: autoland-ui: add UI for landing to Try. (bug 1334225).

Review commit: https://reviewboard.mozilla.org/r/108814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108814/

Comment 2

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review110312

::: autoland/ui/src/components/AutolandController.jsx:151
(Diff revision 1)
>            revisions={data.revisions} />
>          <ActionButtons
>            landable={landable}
>            bug={data.bug}
>            landcallback={this.sendPost} />
> +        <TryChooser />

Should this be here?  Aren't AutolandController and Try completely different?

::: autoland/ui/src/components/TryChooser.css:1
(Diff revision 1)
> +.try-chooser {

Should we use `max-width` for the sake of being responsive?

::: autoland/ui/src/components/TryChooser.css:10
(Diff revision 1)
> +  border-radius: 15px 15px;
> +  padding-bottom: 10px;
> +}
> +
> +.try-chooser > h3 {
> +  width: 100%;

h3 is block aready, right?

::: autoland/ui/src/components/TryChooser.css:11
(Diff revision 1)
> +  padding-bottom: 10px;
> +}
> +
> +.try-chooser > h3 {
> +  width: 100%;
> +  color: white;

Let's use hex values

::: autoland/ui/src/components/TryChooser.jsx:13
(Diff revision 1)
> +    this.state = this.getInitialState();
> +    this.updateStringFromFrame = this.updateStringFromFrame.bind(this);
> +    this.updateStringFromInput = this.updateStringFromInput.bind(this);
> +  }
> +
> +  getInitialState() {

Let's use the pattern I mentioned here:  https://gist.github.com/darkwing/bd7b58c0eb85fe8dd31fea7f6d7bba86

Bonus points if you change my use in AutolandController.jsx.

Comment 3

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review110312

> Should this be here?  Aren't AutolandController and Try completely different?

Dropping this one, as it sounds like we will want this in the same page.

Comment 4

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108814/diff/1-2/

Comment 5

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review110312

> Let's use hex values

will fix

Comment 6

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108812/#review111762

Better Gif link: https://gfycat.com/LankyFarflungArmyant

Comment 7

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review112012

::: autoland/ui/src/components/TryChooser.css:11
(Diff revision 2)
> +  min-width: 80%;
> +  max-width: 1055px;
> +  min-height: 600px;
> +  padding-bottom: 10px;
> +  box-sizing: border-box;
> +  background-color: white;

Let's use hex `#fff` ; I'm not going to comment too much on styles as I plan to redo the "design" once this lands.

::: autoland/ui/src/components/TryChooser.jsx:12
(Diff revision 2)
> +  defaultState = { tryString: '' };
> +
> +  constructor(props) {
> +    super(props);
> +    this.state = this.defaultState;
> +    this.updateStringFromFrame = this.updateStringFromFrame.bind(this);

Do all of these really need to be bound?

::: autoland/ui/src/components/TryChooser.jsx:18
(Diff revision 2)
> +    this.updateStringFromInput = this.updateStringFromInput.bind(this);
> +    this.onLandClick = this.onLandClick.bind(this);
> +    this.onCancelClick = this.onCancelClick.bind(this);
> +  }
> +
> +  resetStateWithUpdates(stateUpdates) {

We should do one of two things here.  First, we don't really need it, since there's only one prop, so we can probably get rid of it....

.... or we should put `resetStateWithUpdates` in another resource and use it as a "mixin" instead of copying it from place to place.

Also, I blogged about this function, and someone brought up that `undefined` (i.e. no stateUpdates are passed in) would break this, so I think in the signature we can do `resetStateWithUpdates(stateUpdates={})`, but I'm not positive.

::: autoland/ui/src/components/TryChooser.jsx:24
(Diff revision 2)
> +    this.setState({ ...this.defaultState, ...stateUpdates });
> +  }
> +
> +  componentDidMount() {
> +    window.addEventListener('message', this.updateStringFromFrame, false);
> +    setTimeout(() => {

`setTimeout` makes me nervous.  At the very least we should document where that number comes from, but is there a way around using it?

::: autoland/ui/src/components/TryChooser.jsx:39
(Diff revision 2)
> +  updateStringFromFrame(event) {
> +    if (event.origin !== 'https://mozilla-releng.net') {
> +      return;
> +    }
> +
> +    if (event.data === '') {

Some people hate on ternaries but I think one here would be cool. 

```
this.resetStateWithUpdates({ tryString: (event.data ? `try: ${event.data}` : '') });
```

::: autoland/ui/src/components/TryChooser.jsx:51
(Diff revision 2)
> +  updateStringFromInput(event) {
> +    this.resetStateWithUpdates({ tryString: event.target.value });
> +  }
> +
> +  onLandClick() {
> +    alert(`Landed to try with string: ${this.state.tryString}`);

We're passing `landcallback` in our other ActionButtons widget; can we pass it here?

::: autoland/ui/src/components/TryChooser.jsx:57
(Diff revision 2)
> +  }
> +
> +  onCancelClick() {
> +    this.elOverlay.classList.remove('try-chooser-overlay-visible');
> +    this.elChooser.classList.remove('try-chooser-visible');
> +    setTimeout(() => this.props.cancelHandler(), 500);

Does this match the CSS animation?  If so we can use `transitionend` event:

https://davidwalsh.name/css-animation-callback

Comment 8

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108814/diff/2-3/

Comment 9

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review112012

> Do all of these really need to be bound?

Apparently so.

> `setTimeout` makes me nervous.  At the very least we should document where that number comes from, but is there a way around using it?

The time isn't important. I was just trying to simulate something like http://underscorejs.org/#defer, since when I run those methods directly react really doesn't like it (i.e. the animation breaks). Do you think there is a better alternative?

Comment 10

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review112430

The only thing I'd add is that if we're animating the Autoland UI in, we could probably automate it out too.

::: autoland/ui/src/components/ActionButtons.jsx:26
(Diff revision 3)
>    render() {
> +    let landButton;
> +    let tryButton;
> +    if (this.props.landable) {
> +      landButton =
> +        <a href="" onClick={this.onLandClick} className="land">

I guess we don't need this `href=""`

::: autoland/ui/src/components/TryChooser.jsx:1
(Diff revision 3)
> +import React from 'react';

Can we add a few stub tests here?  Would be good to start making that a habit.

Comment 11

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108814/diff/3-4/

Comment 12

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review113750

::: autoland/ui/src/components/AutolandController.jsx:54
(Diff revision 4)
>            });
>          });
>        });
>    }
>  
> -  sendPost() {
> +  sendPost = () => {

Why this update?

Comment 13

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review113750

> Why this update?

I realized that we can use the inline functions so that we don't have to bind `this` to every callback function in the constructor.

Comment 14

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review113764

Animation is a bit janky when closing but let's get this in and improve as we focus more on UI.

Comment 15

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108814/diff/4-5/

Comment 16

[Israel Madueme [:imadueme]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108814/diff/5-6/

Comment 17

[David Walsh :davidwalsh]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review114244

Comment 18

[Steven MacLeod [:smacleod]]

Comment on attachment 8832633 [details]
autoland-ui: add UI for landing to Try. (bug 1334225).

https://reviewboard.mozilla.org/r/108814/#review114250

ruber stamp to land.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/automation/conduit/rev/4de2905dfe7d
autoland-ui: add UI for landing to Try. . r=davidwalsh,smacleod