Closed Bug 1350229 Opened 8 years ago Closed 8 years ago

Removing Preview side panel

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.3 - Apr 17
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: gasolin, Assigned: locke12456, Mentored)

References

Details

(Keywords: dev-doc-needed, good-first-bug, Whiteboard: [netmonitor-reserve])

Attachments

(1 file, 2 obsolete files)

Bug 1350229 - Removing Preview side panel.
(deleted), text/x-review-board-request
rickychien
: review+
Details
The Preview side panel seems to duplicate functionality of the Response panel. We should remove that and if the preview feature is still desired it should be merged into the Response panel.
To remove preview panel, you need know how to use react component. Remove devtools/client/netmonitor/shared/components/preview-panel.js, remove the `preview-panel.js` entry in devtools/client/netmonitor/shared/components/moz.build And remove any dependency for `preview-panel.js`, might include styles in netmonitor.css http://searchfox.org/mozilla-central/source/devtools/client/themes/netmonitor.css
Mentor: gasolin
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [netmonitor]
Flags: qe-verify?
Priority: P3 → P2
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Assignee: nobody → locke12456
Mentor: gasolin → rchien
@Bryan: This bug is about removing the Preview side-panel in the Network panel and perhaps merge with the Response side-panel (this panel is only used by HTML doc requests). Chrome does that already and it also eliminates number of side panels. Does it sound ok to you? Should we go ahead or rather file a UX request? Honza
Flags: needinfo?(clarkbw)
Attached patch Bug - 1350229 patch (obsolete) (deleted) — — Splinter Review
Bug 1350229 - Removing Preview side panel r?rickychien
Attachment #8852377 - Flags: review+
Attachment #8852377 - Flags: review+ → review?(rchien)
Status: NEW → ASSIGNED
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
(In reply to Jan Honza Odvarko [:Honza] from comment #2) > @Bryan: This bug is about removing the Preview side-panel in the Network > panel and perhaps merge with the Response side-panel (this panel is only > used by HTML doc requests). Chrome does that already and it also eliminates > number of side panels. Does it sound ok to you? Should we go ahead or rather > file a UX request? No, go ahead with this. It makes a lot of sense to remove this.
Flags: needinfo?(clarkbw)
Comment on attachment 8852377 [details] [diff] [review] Bug - 1350229 patch Review of attachment 8852377 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! Two things: 1) Please remove also netmonitor/shared/components/preview-panel.js file 2) Render the HTML preview within the Response side-panel add #2) The Response side-panel already renders (collapsible/expandlable) sections for various response types (e.g. JSON). It currently renders "Response payload" for HTML requests and the preview should be added as a new section "Preview". Honza
Attached patch Bug-1350229-Removing-Preview-side-panel.patch (obsolete) (deleted) — — Splinter Review
Bug 1350229 - Removing Preview side panel r?rickychien Removed file: devtools/client/netmonitor/src/components/preview-panel.js dependencies on: devtools/client/netmonitor/src/components/moz.build devtools/client/netmonitor/src/components/tabbox-panel.js preview-panel.js had used css class "panel-container". but this class also have many dependency with other sources.(like headers-panel.js)
Attachment #8852790 - Flags: review?(rchien)
Attachment #8852377 - Attachment is obsolete: true
Attachment #8852377 - Flags: review?(rchien)
Comment on attachment 8852790 [details] [diff] [review] Bug-1350229-Removing-Preview-side-panel.patch Review of attachment 8852790 [details] [diff] [review]: ----------------------------------------------------------------- Locke, nice work for the good first bug! Everything you did looks good to me. Let's ship it!
Attachment #8852790 - Flags: review?(rchien) → review+
Keywords: checkin-needed
@Locke: one of the goals of this bug was merging the preview with the Response side-panel (see also comment #5). But there are some UI issues that needs to be discussed first. Can you please file a follow up for this part? Thanks! Honza
Flags: needinfo?(locke12456)
Sure, I will still which on this issue. :) (or another one you can assign to me)
Flags: needinfo?(locke12456)
oh,many thanks Iris and Carsten. I'll check dependencies again.
Flags: needinfo?(locke12456)
Depends on: 1352035
No longer depends on: 1350215
You should remove devtools/client/netmonitor/test/browser_net_html-preview.js as well.
(In reply to Tim Nguyen :ntim from comment #16) > You should remove > devtools/client/netmonitor/test/browser_net_html-preview.js as well. Oh, thanks, I'll check it again.
(In reply to Iris Hsiao [:ihsiao] from comment #11) > sorry had to back this out for eslint failure like > https://treeherder.mozilla.org/logviewer.html#?job_id=87516759&repo=mozilla- > inbound&lineNumber=4276 > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 03d602fd723ad6ff4588c04855884ffa1dee9410 @Locke, this has been backed out. Did you push this again? Honza
Flags: needinfo?(locke12456)
Follow up reported: Bug 1353319 - Render the HTML preview within the Response side-panel Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #19) > (In reply to Iris Hsiao [:ihsiao] from comment #11) > > sorry had to back this out for eslint failure like > > https://treeherder.mozilla.org/logviewer.html#?job_id=87516759&repo=mozilla- > > inbound&lineNumber=4276 > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > > 03d602fd723ad6ff4588c04855884ffa1dee9410 > > @Locke, this has been backed out. Did you push this again? > > Honza Yes,I have pushed commit again and now is awaiting review.
Flags: needinfo?(locke12456)
Comment on attachment 8854298 [details] Bug 1350229 - Removing Preview side panel. https://reviewboard.mozilla.org/r/126226/#review128888 LGTM! I saw your try is green. Let's ship it!
Attachment #8854298 - Flags: review?(rchien) → review+
Keywords: dev-doc-needed
Attachment #8852790 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6b8483bcad11 Removing Preview side panel. r=rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6b8483bcad11
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
Reproduced this with an old Nightly build from 2017-03-24. I can confirm that Preview Panel is no longer displayed (for HTML docs) in the Sidebar panel on latest Nightly 55.0a1 (2017-04-10).
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Depends on: 1393730
(In reply to Fred Lin [:gasolin] from comment #0) > The Preview side panel seems to duplicate functionality of the Response > panel. We should remove that and if the preview feature is still desired it > should be merged into the Response panel. That statement was obviously wrong, as the Response panel only shows the HTML code but not a preview of the response. So, now the panel was removed without having the replacement as of bug 1353319 for it in place. That's leaves a bad UX and people are already missing it[1]. If you plan to replace a feature by a better UI, please make sure both changes land at the same time! Sebastian [1] https://stackoverflow.com/questions/41913076/how-to-get-a-preview-of-the-html-returned-by-a-network-requests/41914758?noredirect=1#comment78787893_41914758
Blocks: 1353319
alcalbg, you can track bug 1353319 status that brings the preview back to the response panel
(In reply to Fred Lin [:gasolin] from comment #34) > alcalbg, you can track bug 1353319 status that brings the preview back to > the response panel I must have missed that, just reading it now. Thanks!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: