Closed Bug 1477597 Opened 6 years ago Closed 6 years ago

Show/update/debug tabs

Categories

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

Product:
DevTools
Component:
about:debugging
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(4 files, 4 obsolete files)

Bug 1477597 - Part 1: Implement a mechanism to dispatch tabs. r=jdescottes
46 bytes, text/x-phabricator-request
jdescottes
: review+
Details | Review
Bug 1477597 - Part 2: Show tabs. r=jdescottes
46 bytes, text/x-phabricator-request
jdescottes
: review+
Details | Review
Bug 1477597 - Part 3: Update UI when tabs are updated. r=jdescottes
46 bytes, text/x-phabricator-request
jdescottes
: review+
Details | Review
Bug 1477597 - Part 4: Add inspect button. r=jdescottes
46 bytes, text/x-phabricator-request
jdescottes
: review+
Details | Review 
This is the splitted bug of bug 1471795.
In this, implement to show/update/debug tabs.
Depends on: 1477596
No longer depends on: 1471795
Blocks: 1477598
No longer blocks: 1477598
Depends on: 1477595
No longer depends on: 1477595
Blocks: 1477603
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.
Also, introduced DebugTarget and Tab class that are similar to Runtime/ThisFirefox class.
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
Attachment #8994098 - Flags: review?(balbeza) → review-
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.
Attachment #8994099 - Flags: review?(jdescottes)
Attachment #8994099 - Flags: review?(balbeza)
Attachment #8994100 - Flags: review?(jdescottes)
Attachment #8994098 - Flags: review?(jdescottes)
Attachment #8994098 - Flags: review-
Attachment #8994097 - Flags: review?(jdescottes)
Moving major about:debugging ng work into milestone 1, leaving m0 for prior bugfix work.
Blocks: remote-debugging-ng-m1
Depends on D2762
Attachment #8994097 - Attachment is obsolete: true
Attachment #8994098 - Attachment is obsolete: true
Attachment #8994099 - Attachment is obsolete: true
Attachment #8994100 - Attachment is obsolete: true
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
Attachment #8997794 - Flags: review+
Comment on attachment 8997795 [details]
Bug 1477597 - Part 2: Show tabs. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8997795 - Flags: review+
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.
Attachment #8997796 - Flags: review+
Comment on attachment 8997797 [details]
Bug 1477597 - Part 4: Add inspect button. r=jdescottes

Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #8997797 - Flags: review+
Pushed by dakatsuka@mozilla.com:
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
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: