Reps modes should be constants

RESOLVED FIXED in Firefox 53

Status

DevTools
Shared Components
P3
normal
RESOLVED FIXED
2 years ago
7 days ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

unspecified
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Honza, what do you think about this ?
Flags: needinfo?(odvarko)
Yes, I like the idea.

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

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → chevobbe.nicolas
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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
Attachment #8812731 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Thanks !

> R+, but please see my inline comments.

Done, and landed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4df1bb338f6d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

7 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.