Closed Bug 1418274 Opened 3 years ago Closed 3 years ago

Responsive Design Mode to use ES6 Classes, prop-types and react-dom-factories

Categories

(DevTools :: Responsive Design Mode, enhancement, P2)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

There are a number of reasons for this:

1. We need to have the modules available for code from devtools-core, which uses them.
2. It begins the process of getting rid of React deprecation warnings.
3. It prepares us to upgrade to React 16.

Versioning info from devtools-core github repo:

"prop-types": "^15.6.0"
"react-dom-factories": "^1.0.2"
Comment on attachment 8929411 [details]
Bug 1418274 - Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/200736/#review206036

Thanks for working on this!

Overall, the code transformations looks good.  However, bug 1333254 and bug 1319597 just landed which modify several of the components here, so we'll need to rebase this work.  In particular, some components are renamed, so that's something to watch out for.

I'd like to double check it once more when that's done.  I am not aware of other refactoring in progress, so there shouldn't be any more conflicts coming while you revise this work.

::: devtools/client/responsive.html/components/Browser.js:23
(Diff revision 1)
>  
>  const FRAME_SCRIPT = "resource://devtools/client/responsive.html/browser/content.js";
>  
> -module.exports = createClass({
> -
> -  /**
> +class Browser extends PureComponent {
> +  static get propTypes() {
> +    return {

This is an important comment about how this component uses data.  Please restore it somewhere, perhaps as a comment on `propTypes`, since they describe the incoming data.

::: devtools/client/responsive.html/components/DprSelector.js:33
(Diff revision 1)
>      title: value,
>      hidden: true,
>      disabled: true,
>    }, value);
>  
> -module.exports = createClass({
> +class DPRSelector extends PureComponent {

This file was renamed to `DevicePixelRatioSelector.js` by bug 1333254, so we'll need to rebase on top of that.  (The component is also renamed similarly.)
Comment on attachment 8929411 [details]
Bug 1418274 - Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/200736/#review206040

(Oops, mis-clicked in MozReview...  Anyway, I'd like see the rebase version, so setting r- for now.)
Attachment #8929411 - Flags: review?(jryans) → review-
Comment on attachment 8929411 [details]
Bug 1418274 - Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/200736/#review206822

::: devtools/client/responsive.html/components/Browser.js:23
(Diff revision 1)
>  
>  const FRAME_SCRIPT = "resource://devtools/client/responsive.html/browser/content.js";
>  
> -module.exports = createClass({
> -
> -  /**
> +class Browser extends PureComponent {
> +  static get propTypes() {
> +    return {

Yup, a side affect of dropping displayName... fixed.
Comment on attachment 8929411 [details]
Bug 1418274 - Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/200736/#review206828

The interdiff is wrong... DprSelector.js no longer exists so you can ignore that section.
Attachment #8929411 - Flags: review?(mratcliffe)
Attachment #8929411 - Flags: review?(mratcliffe)
Could you review this before your PTO?
Flags: needinfo?(jryans)
Comment on attachment 8929411 [details]
Bug 1418274 - Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/200736/#review206942

Thanks for working on this! :) It looks good to go.
Attachment #8929411 - Flags: review?(jryans) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5351572bb1e7
Responsive Design Mode to ES6 Classes, prop-types and react-dom-factories r=jryans
https://hg.mozilla.org/mozilla-central/rev/5351572bb1e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.