1297108 - [meta] Create react component for inspector box-model view

Status: RESOLVED DUPLICATE of bug 1333561

(DevTools :: Inspector, enhancement, P3)

Description

[Ricky Chien [:rickychien] (inactive)]

Inspector box-model view (inspector.xul, computed.js) has been converted into HTML.

Before we begin to implement new box-model design (Bug 1150496), it would be a great benefit if we adopt react as architecture and have a set of separate and reusable components.

Patrick & Honza, we want to hear your opinions about that. How difficult is it to convert inspector.xul, computed.js into react component and what would be a risk we may run into during code refactoring?

Comment 1

[[e-mail]]

As a discussion base, the possible approach in my mind is:

1. add loader method to support load side sub-panel with react
bug 1288347 - add addPanelTab method to load react panel is already have a workable implementation

Current sub panel only support load from xul DOM, but does not support load from DOM or react directly.

2. wrap computed view related DOM (#sidebar-panel-computedview) from inspector.xul to react, located it in `inspector/components/computed.js`, keep minimum change of `inspector/computed` and `inspector/layout`, only change them when necessary so we won't spend too much time on refactor. We'll refactor once we need introduce some new features on them.

3. implement new elements with react and put them into `inspector/components`

Base on analysis of current dom in xul

> (div#sidebar-panel-computedview)
>   div #computedview-toolbar
>     div.devtools-searchbox
>     label#style-checkbox
>   div #computedview-container
>     (box-model)
>     (box-properties)
>   (div#propertyContainer)

The proposed initial react components are:

- computed-panel (the whole panel)
- box-model (correspondent with layout.js)
- searchbar + computed-properties (correspondent with computed.js)
- box-properties (*new)

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Ricky Chien [:rickychien] from comment #0)
> Inspector box-model view (inspector.xul, computed.js) has been converted
> into HTML.
inspector.xul is still being converted right now. This is part of the devtools.html project, and is still ongoing. It's a big refactor that means a lot of the code is moving at the same time.

> Before we begin to implement new box-model design (Bug 1150496), it would be
> a great benefit if we adopt react as architecture and have a set of separate
> and reusable components.
You are correct in that we'd like all of code to use the same UI architecture and technology. In our case, we've decided to invest in React and Redux. So ultimately, we'd like all of our UI code to use those, and share more UI components than we've been able to in the past.
I don't necessarily see this as a blocker for implementing the new box-model designs though. More about this below.

> Patrick & Honza, we want to hear your opinions about that. How difficult is
> it to convert inspector.xul, computed.js into react component and what would
> be a risk we may run into during code refactoring?
So, inspector.xul is already being converted to HTML. Honza is working on converting the last big parts (the splitter) and after that's done, the inspector.xul will become inspector.xhtml, and will contain HTML code.
My concern here is that because this is ongoing, there are a lot of moving parts, and the code is changing all the time. We are converting key UI components from XUL to HTML and, as a result, we sometimes introduce regressions. This is expected as in any large refactoring projects, you're bound to re-introduce bugs for things that worked before, just because of the nature of the work. On top of this, we're sometimes introducing React in some of the UI code we're changing, further adding risk on top.
So, although migrating the box-model widget to React is something we definitely want to do, I don't think now is the right time. It's unfortunately going to add even more risk to an already risky situation. At least that's my opinion on the project right now. I'd really love to be done with the XUL -> HTML refactoring first, and spend the right effort on QA testing and fixing regressions before starting yet another refactor (in that case HTML -> React).

Now, to come back to your question about how difficult it would be, I don't think it would be so hard. But, there are several levels of migration we could consider:
- simple migration from our current JS to React components, keeping the current HTML structure and basic architecture
- or a more profound re-architecture of the box-model view into new React components. This is more of a total rewrite, and therefore has risks of introducing regressions too
- or this + introduce Redux for managing the data model and user actions flow. This is where we want to go ultimately, but to me this would require a rethinking of the inspector as a whole. I don't see a reason to do this just for one small widget (the box-model) in isolation.

So, in that list, option 1 doesn't help us much, we'd have to create new React components later again anyway. Option 2 is better, but again, I don't think now is the right time. Option 3 should be done thinking of the whole inspector, not just the box-model.

Going with option 2 shouldn't be too hard, the coding part should be rather simple. The box-model widget doesn't have that many features anyway, and we have tests that act as a sort of "spec" for us to make sure we don't forget anything. But in these kinds of projects, it's always *after* you're done with the code that it gets complex. That's when you realize that you've introduced regressions that are hard to fix, or edge case race conditions or things like this. So don't underestimate the time it will take.

Furthermore, we should be clear that the box-model is being re-designed and re-thought. I think you're aware of the mockups that Helen made.
The current agreement is that:
- we have a simple box-model widget in the computed-view, sort of similar to what we already have right now,
- and then, we introduce a new "Layout" sidebar panel that contains a richer box-model widget. So in reality we'll have 2 instances of it in the inspector, only one shows more information than the other (for info, the "Layout" panel will host other things like CSS Grid information, CSS Flexbox information, ..., see https://projects.invisionapp.com/share/3X87NEBYH#/screens/179720294 ).

So, in summary:
- I don't think we should start a React refactor of the box-model widget just now,
- The new features of the box-model (like getting information about z-index, stacking context element, container element, box-sizing, ...) aren't really blocked, imo, on refactoring to React (there is a bit of server-side actor work to be done too anyway),
- Maybe we should start with working on the new Layout panel, and since it's a new one, we can more easily use React right from the start. 

Sorry for the long reply. I hope this helps.

Comment 3

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

I think Patrick summarized it well and I agree that converting things into React components needs to have good reasons (not just making the arch nicer). 

Honza

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Thanks for your detail information!

I totally agreed with that the plan of converting things to React and new box-model design is not so urgency at the moment and we will keep contributing HTML refactoring as our first priority.

thanks a lot!