1312178 - Reps modes should be constants

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

At the moment, we declare the mode of the string with a string, and there isn't a shared place where the supported modes are declared. This is error prone , or at least, could confuse someone, since you can pass a concise mode ( such things exists in the inspector I think ) and wonder why it doesn't render what they expect.
IMO it would be nice to have the modes as constants and have the propType of the Reps requiring the mode to be one of the constant.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Honza, what do you think about this ?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, I like the idea.

Should we put it into rep-utils.js file or create new one? 

Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

I think it makes sense to have a dedicated file for this, something like rep-modes.js .
I'll do this when the several reps bugs I work on lands.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1312178 - Make Reps modes constants;

Edit tests and component creation accordingly.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

TRY looks good for this : https://treeherder.mozilla.org/#/jobs?repo=try&revision=50a5b47756566e39b33c5d95ea351c6319bcff99

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8812731 [details]
Bug 1312178 - Make Reps modes constants;

https://reviewboard.mozilla.org/r/94374/#review94586

Looks good!

R+, but please see my inline comments.

Honza

::: devtools/client/shared/components/reps/grip.js:202
(Diff revision 1)
>      },
>  
>      render: function () {
>        let object = this.props.object;
>        let props = this.safePropIterator(object,
> -        (this.props.mode == "long") ? 10 : 3);
> +        (this.props.mode == MODE.LONG) ? 10 : 3);

Use === operator

::: devtools/client/shared/components/reps/grip.js:205
(Diff revision 1)
>        let object = this.props.object;
>        let props = this.safePropIterator(object,
> -        (this.props.mode == "long") ? 10 : 3);
> +        (this.props.mode == MODE.LONG) ? 10 : 3);
>  
>        let objectLink = this.props.objectLink || span;
> -      if (this.props.mode == "tiny") {
> +      if (this.props.mode == MODE.TINY) {

Use === operator

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8812731 [details]
Bug 1312178 - Make Reps modes constants;

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Thanks !

> R+, but please see my inline comments.

Done, and landed

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4df1bb338f6d
Make Reps modes constants; r=Honza

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4df1bb338f6d