Closed
Bug 1323454
Opened 7 years ago
Closed 7 years ago
Network panel: integrate HTTP Status code with MDN
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox54 verified)
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: Honza, Assigned: kenweilee, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [netmonitor-reserve])
Attachments
(2 files, 7 obsolete files)
36.76 KB,
image/png
|
Details | |
5.33 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
See also bug 1323442 The "Headers" side panel available in the Network panel renders HTTP status code. We should display a link next to it pointing to related MDN page (if available) Comments: - The UI should be consistent with the Console panel and HTTP Headers (bug 1323442) - We can render just simple "[learn more]" link (no quotes) next to the Status code - List of MDN pages related to status code should be hardcoded. - See attached mockup showing the link next to the status code field. Honza
Reporter | ||
Updated•7 years ago
|
Whiteboard: [netmonitor][triage]
Reporter | ||
Comment 1•7 years ago
|
||
MDN page for HTTP status codes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status Honza
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Updated•7 years ago
|
Whiteboard: [netmonitor][triage] → [netmonitor]
Comment 2•7 years ago
|
||
Hi Honza, should this bug be added to the MVP or Reserve backlog?
Reporter | ||
Comment 3•7 years ago
|
||
Reserve backlog Honza
Mentor: odvarko
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(odvarko)
Keywords: good-first-bug
Priority: -- → P3
Updated•7 years ago
|
Whiteboard: [netmonitor] → [netmonitor-reserve]
I'm new to this stuff and trying to get my foot in the door. How would we go about implementing this?
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Parker from comment #4) > I'm new to this stuff and trying to get my foot in the door. How would we go > about implementing this? Sounds great! The UI implementation of the 'Headers' side panel is based on React and so, some knowledge of the library is needed. Here are some comments that should help when implementing this feature. 1) The (React) component representing the Headers side panel is implemented here: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/shared/components/headers-panel.js 2) Rendering of the Status code is done in HeadersPanel.render() method https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/devtools/client/netmonitor/shared/components/headers-panel.js#147 3) The '[more link]' should be displayed next to the status code and text (after the 'input' element, see the render method). Here is a screenshot showing how the '[learn more]' link looks like in the Console panel: https://bug1323442.bmoattachments.org/attachment.cgi?id=8818543 The Headers panel should use the same design. 4) Not all status codes are documented on MDN so, we should have hard-coded list of documented statuses in haders-panel.js file. The link should appear only for statuses that are actually documented. MDN page for status codes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status MDN page for status 200 OK: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200 Honza
Reporter | ||
Comment 6•7 years ago
|
||
Btw. there is similar bug for HTTP headers (bug 1320233) Honza
Updated•7 years ago
|
Priority: P3 → P4
Hi guys, I am learning to contribute to Mozilla. As [:Honza] pointed, I made change in headers-panel.js. The items are displayed using this way in render(): input({ className: "tabpanel-summary-value textbox-input devtools-monospace", readOnly: true, value: `${status} ${statusText}`, I tried adding a link beside it by: input({ className: "tabpanel-summary-value textbox-input devtools-monospace", readOnly: true, value: "[Learn More]".link("www.link.com"),}), But, the value is displayed as HTML tags. I think because it is within input() and text-box input. How do I find other classes which can display HTML content within the dev-tools? Thanks in advance for any help.
Comment 8•7 years ago
|
||
Saghan, thanks for interesting in contribute! You have a good start to trace the code and successfully find the `renderSummary` function https://github.com/mozilla/gecko-dev/blob/master/devtools/client/netmonitor/shared/components/headers-panel.js#L81 You can import and add extra link element(`a`) after the input element to host the `[Learn More]` link. You can refer `headerDocURL` in same file to find out how to construct the link element. Then you could check the style and make sure UI shows correctly. (You might need apply flex flexible style to auto adjust the input field width) You can reach us at IRC#devtools or just use bugzilla's needinfo if you need help
Flags: needinfo?(saghan99)
Comment 9•7 years ago
|
||
Once bug 1339686 is landed, you can use `MdnLink({ link: statusURL })` to render MDN status code link
Assignee | ||
Comment 10•7 years ago
|
||
Hi, This is my first time trying my hand at this and I've stumbled into a problem at the last leg of this bug. I have the `[Learn More]` link implemented and it's working as desired, the link only appearing if there's a valid MDN page. The only issue I have now is that I can't quite seem to figure out how to style it. I've looked at the `headerDocURL` section to try and use it but part of that code relies on the props parameter to renderValue() but render() has no such parameter.
Flags: needinfo?(gasolin)
Comment 11•7 years ago
|
||
Hi Ken, thanks for take it a look! please put your work-in-progress patch onto bugzilla (via tap `Add an attachment` link on this page) and I can help check what should we do for the next step. The basic idea is we have `Rep` and `MDNLink` element inside of `.treeValueCellDivider` class, and we should put `MDNLink` element always align to the right side of the space. So the result might be add a rule such as ``` .treeValueCellDivider > learn-more-link { (... align MDNLink to right) } ``` You should also check headers panel's CSS and learn how it shows [Learn More] link besides the value.
Flags: needinfo?(gasolin) → needinfo?(kenweilee)
Assignee | ||
Comment 12•7 years ago
|
||
Hi Fred, I have attached the WIP patch. As you will see, I did use `treeValueCellDivider` so the text is aligned to the right. However, I still need to make it blue and change it so that it only one line (it currently uses two lines). I am not really sure how to use Rep, any help would be appreciated.
Flags: needinfo?(kenweilee) → needinfo?(gasolin)
Comment 13•7 years ago
|
||
const { getHeadersURL } = require("../../utils/mdn-utils"); +const { getHTTPStatusCodeURL } = require("../../utils/mdn-utils"); You can combine these 2 lines as ``` const { getHeadersURL, getHTTPStatusCodeURL, } = require("../../utils/mdn-utils"); ``` +function getHTTPStatusCodeURL(statusCode) { + let idx = SUPPORTED_HTTP_CODES.findIndex(item => You can use simpler `SUPPORTED_HTTP_CODES.indexOf(statusCode)` instead of findIndex, since there's no capital issues in status code. div + className: "treeValueCellDivider"}, + statusCodeDocURL ? MDNLink({ + url: statusCodeDocURL, + }) : null + ), Sorry the comment 11 CSS style is based on header's condition. For this case, instead of wrapping extra `div`, you can use MDNLink directly ``` statusCodeDocURL ? MDNLink({ url: statusCodeDocURL, }) : null ``` In this case, the MDNLink element should be near `devtools-button`. And you can use inspector to debug the devtools netmonitor as normal web page by following https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
Flags: needinfo?(gasolin)
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → kenweilee
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•7 years ago
|
||
Hi Fred, Using the debugger, I have found out why the styling is not applied: ``` .treeTable .treeValueCell .learn-more-link { color: var(--theme-highlight-blue); cursor: pointer; margin 0 5px; } .treeTable .treeValueCell .learn-more-link:hover { text-decoration: underline; } ``` I'm wondering if it'd be a good idea to remove `.treeTable .treeValueCell` and have the rule moved to netmonitor.css so that any `.learn-more-link` would be managed by the same style or if I should copy the style separately into netmonitor.css (where .tabpanel-summary-container and .headers-summary are found?)
Flags: needinfo?(gasolin)
Comment 15•7 years ago
|
||
Good catch! The styles you found in tree-view.css is the basic style, you don't need to touch it http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tree/tree-view.css#87 You could just add new rule in netmonitor.css to overwrite the styles for this case.
Flags: needinfo?(gasolin)
Assignee | ||
Comment 16•7 years ago
|
||
Hi, I've attached the rough draft of the finalized patch for review.
Attachment #8839303 -
Attachment is obsolete: true
Attachment #8839954 -
Flags: review?(odvarko)
Reporter | ||
Comment 17•7 years ago
|
||
I can't apply the patch on latest m-c. Can you please rebase? Honza patching file devtools/client/netmonitor/shared/components/headers-panel.js Hunk #1 FAILED at 11 Hunk #2 FAILED at 188 2 out of 2 hunks FAILED -- saving rejects to file devtools/client/netmonitor/shared/components/headers-panel.js.rej patching file devtools/client/netmonitor/utils/mdn-utils.js Hunk #1 FAILED at 120 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/utils/mdn-utils.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory
Assignee | ||
Comment 18•7 years ago
|
||
Hi Honza, Sorry, but could you elaborate on that? I'm very new to the process and Mercurial. I read that as needing to get the latest mozilla-central and making a new patch file with my changes. Is there an easier way to do that than manually copying what I added, updating to latest, and putting the code back in before making a new patch file?
Assignee | ||
Comment 19•7 years ago
|
||
Hi Honza, I fiddled around with Mercurial and tried to reset it to the latest before adding the fix again. I hope this one works.
Attachment #8839954 -
Attachment is obsolete: true
Attachment #8839954 -
Flags: review?(odvarko)
Attachment #8840240 -
Flags: review?(odvarko)
Comment 20•7 years ago
|
||
The code looks good, but the [learn more] link should be shown near the status code as the mockup provided by Honza https://bugzilla.mozilla.org/attachment.cgi?id=8818568 And please * put CSS styles under /* Headers tabpanel */ section so we know this changes are related to Header's panel * remove /* Learn more link */ since the class already explains all
Comment 21•7 years ago
|
||
Add `flex-grow: 1;` into `.tabpanel-summary-container .learn-more-link` rule and set input field width based on its value (304 Not modified -> width: 60px, 200 OK -> width: 40px) could do the trick. Though it will be great if this can be solved by pure CSS.
Reporter | ||
Comment 22•7 years ago
|
||
Comment on attachment 8840240 [details] [diff] [review] final.patch Review of attachment 8840240 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me too. Please see my inline comments. Also, please set correct commit message: Bug 1323454 - Network panel: integrate HTTP Status code with MDN; r=Honza,gasolin Thanks for working on this! Honza ::: devtools/client/netmonitor/shared/components/headers-panel.js @@ +12,4 @@ > } = require("devtools/client/shared/vendor/react"); > const { L10N } = require("../../utils/l10n"); > const { writeHeaderText } = require("../../utils/request-utils"); > +const { nit: remove whitespace at the end of the line ::: devtools/client/netmonitor/utils/mdn-utils.js @@ +95,5 @@ > + "406", > + "410", > + "412", > + "451", > + "500", nit: remove whitespaces at the end of the line
Attachment #8840240 -
Flags: review?(odvarko)
Reporter | ||
Comment 23•7 years ago
|
||
(In reply to Ken Lee from comment #19) > Created attachment 8840240 [details] [diff] [review] > final.patch > > Hi Honza, > > I fiddled around with Mercurial and tried to reset it to the latest before > adding the fix again. Yep, works fine now, thanks! Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 24•7 years ago
|
||
Hi, Here's the patch with the necessary fixes as well as making it look like the mock-up. It builds on top of final.patch, I hope that's alright/the proper way to do this.
Attachment #8840677 -
Flags: review?(odvarko)
Reporter | ||
Comment 25•7 years ago
|
||
Excellent, thanks! I tested this and looks good to me. Having two patches is ok. But, the commit message is still not set (see my comment #22) Commit message for the first patch might be somethings like as follows: Bug 1323454 - Render learn more link for HTTP Status code; r=Honza,gasolin ...and the second Bug 1323454 - Fix learn more link layout; r=Honza,gasolin Honza
Flags: needinfo?(kenweilee)
Reporter | ||
Comment 26•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8830849df6d11f72e8294b445b8f09c35fa33790 Honza
Assignee | ||
Comment 27•7 years ago
|
||
Hi Honza, I used hg histedit to change the commit messages. When I tried to push it said it'd create a new remote head and aborted the operation. Should I go ahead and have it create a new remote head? It also suggested I try merging before the push, but I didn't want to screw anything up by doing something I'm unsure of. Also I'm not really sure if there's something I should be doing with the treeherder link.
Flags: needinfo?(kenweilee) → needinfo?(odvarko)
Reporter | ||
Comment 28•7 years ago
|
||
> Also I'm not really sure if there's something I should be doing with the treeherder link. The Try push (comment #26) says that there is an Eslint error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8830849df6d11f72e8294b445b8f09c35fa33790&selectedJob=79971429 So, this needs to be fixed: R | /home/worker/checkouts/gecko/devtools/client/netmonitor/shared/components/headers-panel.js:191:1 | Line 191 exceeds the maximum line length of 90. (max-len) I am using mercurial queues [1] and all you need to do to set the commit message is to push the patch into hg queue and use: hg qref --message "<commit-message>" (this will set commit message for the last applied patch in the queue) [1] https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues -- As soon as the eslint and commit message are fixed we'll mark this bug as 'checkin-needed' and somebody will land the patch(es) for us. Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 29•7 years ago
|
||
Changed commit message of first final patch.
Attachment #8840240 -
Attachment is obsolete: true
Assignee | ||
Comment 30•7 years ago
|
||
Fixed commit message of final2.patch
Attachment #8840677 -
Attachment is obsolete: true
Attachment #8840677 -
Flags: review?(odvarko)
Assignee | ||
Comment 31•7 years ago
|
||
Hi, This third patch fixes the line length. I have changed the commit messages of the previous 2 patches and re-uploaded them as well.
Attachment #8841602 -
Flags: review?(odvarko)
Reporter | ||
Comment 32•7 years ago
|
||
I am still not seeing the commit message in your patches so, I merged all of them into one and set it. Please see the header at the top of the patch. Especially the line: Bug 1323454 - integrate HTTP Status code with MDN; r=Honza (it's the commit message) Here is new try push so, we know that there are no problems before it's landed. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=200e589f599136885e385c87d9ad405e6eb7ced5 See more info about Try server here: https://wiki.mozilla.org/ReleaseEngineering/TryServer Let's see how it goes! :-) Honza
Attachment #8841616 -
Flags: review?(odvarko)
Reporter | ||
Updated•7 years ago
|
Attachment #8841598 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8841599 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8841602 -
Attachment is obsolete: true
Attachment #8841602 -
Flags: review?(odvarko)
Assignee | ||
Comment 33•7 years ago
|
||
I was using hg commit to set the message, but I guess that doesn't work for patches. For the future, I think I'll use the Mercurial Queue; thanks for showing it to me!
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•7 years ago
|
Attachment #8841616 -
Flags: review?(odvarko) → review+
Comment 34•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52078590e97f integrate HTTP Status code with MDN; r=Honza
Keywords: checkin-needed
Comment 35•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52078590e97f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•7 years ago
|
Iteration: --- → 54.3 - Mar 6
Priority: P4 → P1
QA Contact: ciprian.georgiu
Comment 36•7 years ago
|
||
This issue is verified fixed on latest Aurora 54.0a2 (2017-03-06) using Windows 10 x64 and Mac OS X 10.11.6. I've verified this based on the info from comment 0, and I can confirm that the [Learn More] link from the Headers side panel is working as expected.
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 37•7 years ago
|
||
Docs updates here: https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor?document_saved=true#Headers
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Flags: needinfo?(saghan99)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•