Refactor LayoutView to a Box Model component

RESOLVED FIXED in Firefox 51

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Summary: Refactor layoutjs to components/box-model.js → Refactor layout.js to components/box-model.js
Summary: Refactor layout.js to components/box-model.js → Refactor LayoutView to a Box Model component
Posted patch 1302496.patch [1.0] (obsolete) — Splinter Review
Attachment #8791038 - Flags: review?(jdescottes)
Julian, this move to a box model component is to make room for making a new Layout view for the css grid and other layout tools. I could see possible arguments for "box-model-" or "boxmodel-". I don't really have preferences, but it can be easily changed.
Comment on attachment 8791038 [details] [diff] [review]
1302496.patch [1.0]

Review of attachment 8791038 [details] [diff] [review]:
-----------------------------------------------------------------

My only concern is about using the components folder here. As we start using react in the inspector, I think it would be better to reserve the components folder for components actually using react? So maybe move it to computed/ or a new boxmodel/ folder? Not blocking the review on this though, so up to you.

R+ with a green try (two trivial tests to fix browser_computed_search-filter_clear.js & browser_computed_search-filter.js)

As a bonus, if you want to cleanup some more references to "layout-view" or "layout view", you can look in:
- devtools/server/actors/highlighters.js
- devtools/server/actors/inspector.js
- devtools/server/actors/styles.js
- devtools/client/inspector/inspector-panel.js
- devtools/client/inspector/components/test/head.js

Thanks for doing this by the way, was never sure how to refer to this panel before!

::: devtools/client/inspector/components/box-model.js
@@ +603,5 @@
>    update: function () {
>      let lastRequest = Task.spawn((function* () {
>        if (!this.isViewVisibleAndNodeValid()) {
>          this.wrapper.hidden = true;
> +        this.inspector.emit("boxmodel-updated");

Was there a good reason to use boxmodel-updated rather than boxmodelview-updated (which seems like the natural translation from layoutview-updated)?

::: devtools/client/locales/en-US/layoutview.dtd
@@ +13,5 @@
>    - documentation on web development on the web. -->
>  <!-- LOCALIZATION NOTE (*.tooltip): These tooltips are not regular tooltips.
>    -  The text appears on the bottom right corner of the layout view when
>    -  the corresponding box is hovered. -->
> +<!ENTITY boxModelTitle            "Box Model">

I would skip the string update here. I will migrate all the strings in layoutview.dtd in Bug 1294186, I will make sure to switch to a "BoxModel" compliant name there.

But in the meantime, no need to create additional work for localization teams.
Attachment #8791038 - Flags: review?(jdescottes) → review+
Attachment #8791038 - Attachment is obsolete: true
Attachment #8792264 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/639635348b41780875198e5f702d5714078deeee
Bug 1302496 - Refactor LayoutView to a Box Model component r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/639635348b41
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.