1477597 - Show/update/debug tabs

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P1)

Description

[Daisuke Akatsuka (:daisuke)]

This is the splitted bug of bug 1471795.
In this, implement to show/update/debug tabs.

Comment 1

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 1: Implement updateTabs action.

Review commit: https://reviewboard.mozilla.org/r/258712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258712/

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 2: Show tabs.

Review commit: https://reviewboard.mozilla.org/r/258714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258714/

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 3: Add debug button to tab list.

Review commit: https://reviewboard.mozilla.org/r/258716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258716/

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 4: Update tab list ui when the actual tabs were changed.

Review commit: https://reviewboard.mozilla.org/r/258718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258718/

Comment 5

[Daisuke Akatsuka (:daisuke)]

I have updated the patches. The different points from bug 1471795 are followings:

* Use action/reducer for tabs.
* Change React component to composition style from inheritance.
* Use CSS grid instead of flex.

Comment 6

[Daisuke Akatsuka (:daisuke)]

Also, introduced DebugTarget and Tab class that are similar to Runtime/ThisFirefox class.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994097 [details]
Bug 1477597 - Part 1: Implement updateTabs action.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258712/diff/1-2/

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994098 [details]
Bug 1477597 - Part 2: Show tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258714/diff/1-2/

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994099 [details]
Bug 1477597 - Part 3: Add debug button to tab list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258716/diff/1-2/

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994100 [details]
Bug 1477597 - Part 4: Update tab list ui when the actual tabs were changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258718/diff/1-2/

Comment 11

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994097 [details]
Bug 1477597 - Part 1: Implement updateTabs action.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258712/diff/2-3/

Comment 12

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994098 [details]
Bug 1477597 - Part 2: Show tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258714/diff/2-3/

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994099 [details]
Bug 1477597 - Part 3: Add debug button to tab list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258716/diff/2-3/

Comment 14

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994100 [details]
Bug 1477597 - Part 4: Update tab list ui when the actual tabs were changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258718/diff/2-3/

Comment 15

[Belén [:ladybenko]]

Comment on attachment 8994098 [details]
Bug 1477597 - Part 2: Show tabs.

https://reviewboard.mozilla.org/r/258714/#review266394

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.css:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

We're not using grid as we should. Let's take advantage of grid gaps, and fraction units. Here's a codepen with an example: https://codepen.io/ladybenko/pen/XBgwVw

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.css:17
(Diff revision 3)
> +  height: 36px;
> +  width: 36px;
> +}
> +
> +.debug-target-item__info {
> +  min-width: 0;

What is this used for?

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.css:14
(Diff revision 3)
> +.debug-target-list__title {
> +  margin-block-start: 10px;
> +  margin-block-end: 10px;
> +}
> +
> +.debug-target-list__ul {

I don't think an html element name should be part of a class name. If using `list` seems redundant, how about changing the first one `debug-target-list` for just `debug-targets` and using `debug-targets__list` for the `ul` ?

::: devtools/client/aboutdebugging-new/src/debugtargets/debug-target.js:1
(Diff revision 3)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Same comment than in a previous patch re: inheritance of components

Comment 16

[Belén [:ladybenko]]

Comment on attachment 8994098 [details]
Bug 1477597 - Part 2: Show tabs.

https://reviewboard.mozilla.org/r/258714/#review266398

Thanks! I left some comments re: grid and the use of inheritance in components.

Comment 17

[:Harald Kirschner :digitarald]

Moving major about:debugging ng work into milestone 1, leaving m0 for prior bugfix work.

Comment 18

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 1: Implement a mechanism to dispatch tabs. r=jdescottes

Comment 19

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 2: Show tabs. r=jdescottes

Depends on D2762

Comment 20

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 3: Update UI when tabs are updated. r=jdescottes

Depends on D2763

Comment 21

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477597 - Part 4: Add inspect button. r=jdescottes

Depends on D2764

Comment 22

[Phabricator Automation]

Comment on attachment 8997794 [details]
Bug 1477597 - Part 1: Implement a mechanism to dispatch tabs. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

https://phabricator.services.mozilla.com/D2762

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8997795 [details]
Bug 1477597 - Part 2: Show tabs. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8997796 [details]
Bug 1477597 - Part 3: Update UI when tabs are updated. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8997797 [details]
Bug 1477597 - Part 4: Add inspect button. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceac9b538854
Part 1: Implement a mechanism to dispatch tabs. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2ca07d4257
Part 2: Show tabs. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e11284f64b
Part 3: Update UI when tabs are updated. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b075bd9b5e
Part 4: Add inspect button. r=jdescottes

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ceac9b538854
https://hg.mozilla.org/mozilla-central/rev/2a2ca07d4257
https://hg.mozilla.org/mozilla-central/rev/e2e11284f64b
https://hg.mozilla.org/mozilla-central/rev/39b075bd9b5e