1654218 - Use a single grid layout for the devtools fission preference popup

Status: RESOLVED FIXED

(DevTools :: General, task, P5)

Description

[Julian Descottes [:jdescottes]]

See https://phabricator.services.mozilla.com/D84197#inline-480720

The devtools preference popup is using two grid containers:

• one for the header
• one for each preference list item

We could have a single grid layout using subgrid, which could allow to align the toggle and reset buttons.

The popup is declared at https://searchfox.org/mozilla-central/source/devtools/client/devtools-fission-prefs.js

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

setting this as a good-first-bug if someone wants to play with subgrid :)

Comment 2

[Cody Welsh]

Hi,

I'd love to start contributing to DevTools; this looks like it may be a good candidate. I'll have to read up a bit on documentation (and subgrid) to make a proper submission, but feel free to suggest anything else I might need to be aware of in the meantime. :)

Thanks! I'll prod for any questions that may arise.

Comment 3

[Cody Welsh]

Sorry for the delay! I successfully enveloped the whole thing in a nicely-aligned grid with associated subgrids; the only thing I need to do now is prime myself with regards to differences between git and hg, as I have never used Mercurial before now (and make sure my code meets standards with a once-over, of course). I also probably need a Phabricator account, if I remember correctly.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Hey Cody, this is great!
You can read https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html to get some mercurial knowledge
and https://firefox-source-docs.mozilla.org/devtools/contributing/making-prs.html?highlight=moz%20phab to see how to push your patch to review.
Hope this helps, don't hesitate to ask any question :)

Comment 5

[Cody Welsh]

Hi, Nicolas,

Sounds good! I did have one (potentially out of scope?) question on my mind - is there a reason we are explicitly setting each individual style property of the nodes, instead of assigning them iteratively from an Object of some sort? (E.g., instead of container.style.x = "y", we have some object composed of { x: "y", foo: "bar", ... } which we then iterate upon.)

Otherwise, I'll submit a PR as soon as I can! Thanks. I'll ask for anything else that I think of. :)

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Cody Welsh from comment #5)

Hi, Nicolas,

Sounds good! I did have one (potentially out of scope?) question on my mind - is there a reason we are explicitly setting each individual style property of the nodes, instead of assigning them iteratively from an Object of some sort? (E.g., instead of container.style.x = "y", we have some object composed of { x: "y", foo: "bar", ... } which we then iterate upon.)

Otherwise, I'll submit a PR as soon as I can! Thanks. I'll ask for anything else that I think of. :)

Since I'm the one who wrote the initial version, I can answer here.
There's no specific reason, I used the simplest implementation I had in mind since this popup was only meant for internal development.
Feel free to change it :)

If you do so, I suggest splitting the cosmetic changes from the actual bug fix (ie grid implementation) in 2 separate commits, it will be easier to review.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 8

[Cody Welsh]

(In reply to Julian Descottes [:jdescottes] from comment #6)

Since I'm the one who wrote the initial version, I can answer here.
There's no specific reason, I used the simplest implementation I had in mind since this popup was only meant for internal development.
Feel free to change it :)

If you do so, I suggest splitting the cosmetic changes from the actual bug fix (ie grid implementation) in 2 separate commits, it will be easier to review.

Okay, sounds reasonable to me! Since it's so simple to change, I'll go ahead and do that as well. Thanks for the input!

Comment 9

[Cody Welsh]

Attachment: Bug 1654218 - single grid layout in Fission popup. r=jdescottes,nchevobbe!

Wraps the header container and the preferences list in a grid layout
that is meant to be a "single source of truth" for nested grids via
subgrid.

Comment 10

[Cody Welsh]

Attachment: Bug 1654218 - Consolidate style assignments into objects. r=nchevobbe,jdescottes!

Reduces explicit "elem.style.x = 'y' assignments by consolidating them
into associated Objects, and iterating on those to assign them to their
proper elements (with a helper function)."

Depends on D86866

Comment 11

[Cody Welsh]

Finally submitted to Phab - I had some issues related to my present lack of Mercurial comfort, but I eventually figured it out. Definitely asking about it next time I get stuck, though! Haha.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5007c2998e6
single grid layout in Fission popup. r=nchevobbe

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f5007c2998e6

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e45cf0154d7f
Consolidate style assignments into objects. r=nchevobbe,jdescottes

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e45cf0154d7f