Closed Bug 1415211 Opened 7 years ago Closed 6 years ago

XHR labels are not aligned

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: tim.xie, Mentored)

References

(
URL
)

Details

(Keywords: good-first-bug)

Attachments

(4 files, 5 obsolete files)

Screen Shot 2017-11-07 at 17.04.06.png
124.24 KB, image/png
Details
Patch applied
228.82 KB, image/png
Details
Screenshot at Feb 28 10-03-00.png
77.49 KB, image/png
Details
Bug 1415211 - Realigned XHR label, set property margin-inline-start of XHR to 0;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
Steps to reproduce:
1. Go to https://console-html-network-requests.glitch.me/
2. Open the console
3. Make sure the "XHR" filter is on
4. Click on the "Trigger Network requests" button
5. You should see 5 XHR requests (resp. GET, OPTIONS, PUT, POST, DELETE)

Expected results:
All the requests are laid-out nicely

Actual results:
The XHR labels are not aligned, which makes the console looks messy (see attachment)

---

I think this can be solved by adding a fixed width in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/devtools/client/themes/webconsole.css#981-983 , so the method all have the same size. 
The downsize is that if we only have GET requests, it can feels like the "GET" and "XHR" elements are a bit disconnected.

The other solution would be to place the XHR label before the method name, which could be done in http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js#101, by inverting `method` and `xhr`
URL: https://console-html-network-requests...
Has STR: --- → yes
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [console-html][triage]
Hi, is this issue still available.  If so, I can take it?
(In reply to Robert Pirritano from comment #1)
> Hi, is this issue still available.  If so, I can take it?

Yes it is, please do :)
Assignee: nobody → rpirritano
Status: NEW → ASSIGNED
Don't hesitate to ask me anything on that
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Don't hesitate to ask me anything on that

Thanks, will do!
(In reply to Robert Pirritano from comment #4)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> > Don't hesitate to ask me anything on that
> 
> Thanks, will do!

You can come in our slack https://devtools-html-slack.herokuapp.com/ if you want to chat :)
Attached patch tip.patch (obsolete) — — Splinter Review 
I used the solution of placing the XHR label before the method name
Attachment #8927686 - Flags: review?(nchevobbe)
Comment on attachment 8927686 [details] [diff] [review]
tip.patch

Hello, thanks for working on this.
Sadly, the patch you uploaded seems invalid.

Furthermore, we are trying to not use splinter review (where you upload patch file to bugzilla) anymore.

Would you mind having a look at http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html and try to push your patch to mozreview ?

Thanks !
Attachment #8927686 - Flags: review?(nchevobbe) → review-
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Comment on attachment 8927686 [details] [diff] [review]
> tip.patch
> 
> Hello, thanks for working on this.
> Sadly, the patch you uploaded seems invalid.
> 
> Furthermore, we are trying to not use splinter review (where you upload
> patch file to bugzilla) anymore.
> 
> Would you mind having a look at
> http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> install.html and try to push your patch to mozreview ?
> 
> Thanks !

Oh no, so sorry, this is my first time working with a patch.  Definitely, I will look at the link you provided and use mozreview. Sorry for the inconvenience!
No worries ;) Thank you for working on this
Attachment #8927686 - Attachment is obsolete: true
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review205304

Thanks for working on that Robert !
Unfortunately, the patch looks weird since it shows the file as "new" when we should only see the `xhr` move.
Can you look at it and see if you can fix this ?

Also, something I did not told you, we need to make sure the tests still pass.
To run our component test you can do :
`cd devtools/client/webconsole && npm install` and then `npm test`.
Attachment #8928319 - Flags: review?(nchevobbe) → review-
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review205304

oh that is wierd, since I only changed that in the file, let me check it out and I will also run the component tests.
Whiteboard: [console-html][triage]
Attached image Patch applied — 
Honza, does this look good to you ?
I like the change, it only seems to reveal a minor an alignment issue
Flags: needinfo?(odvarko)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Created attachment 8931291 [details]
> Patch applied
> 
> Honza, does this look good to you ?
> I like the change, it only seems to reveal a minor an alignment issue
Looks good, but can we also properly v-center the toggle-arrow icon?
I think that all we need to do is use:
align-items: center;
... for the message flex layout.

Honza


diff --git a/devtools/client/themes/webconsole.css b/devtools/client/themes/webconsole.css
--- a/devtools/client/themes/webconsole.css
+++ b/devtools/client/themes/webconsole.css
@@ -24,16 +24,17 @@ a {
 *:visited { }

 * {
   box-sizing: border-box;
 }

 .message {
   display: flex;
+  align-items: center;
   padding: 0 7px;
   width: 100%;
   box-sizing: border-box;
 }

 .message > .prefix,
 .message > .timestamp {
   flex: none;
Flags: needinfo?(odvarko)
Comment on attachment 8928319 [details]
Bug 1415211 - XHR label are not aligned.  Inverted the method name and xhr; :nchevobbe

https://reviewboard.mozilla.org/r/199522/#review207834

Okay, we need to fix the alignment issue, and then it should be good !
Attachment #8928319 - Flags: review?(nchevobbe) → review+
Hi, 
Is this bug still open? There doesn't seem to be anything left to do, but no one is here for finish it up...

Tim
Flags: needinfo?(nchevobbe)
Yes, this bug is still open.
Robert, would you have the time to finish this up ? If not, do you mind if someone else does ?

Tim, would you want to finish it ?
Flags: needinfo?(tim.xie)
Flags: needinfo?(rpirritano)
Flags: needinfo?(nchevobbe)
Sure, Tim can finish it since I wont be able to get to it for a while :)
Flags: needinfo?(rpirritano)
Summary: XHR label are not aligned → XHR labels are not aligned
Hi Nicolas, 
I would like to finish this bug! (or what's left of it) If I understood this correctly, the only part left is this: 

(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> Looks good, but can we also properly v-center the toggle-arrow icon?
> I think that all we need to do is use:
> align-items: center;
> ... for the message flex layout.
> 
> Honza

Tim
Flags: needinfo?(tim.xie) → needinfo?(nchevobbe)
Thanks for working on this Tim !
So, the XHR label is now aligned with the arrow and the text, but other messages are off (see non-xhr requests and groups).

I think here we should only act on the xhr label to align it with the other parts of a message
Flags: needinfo?(nchevobbe)
Attachment #8928319 - Attachment is obsolete: true
XHR label is a bit taller than the line height, so I reduced the vertical padding this time. The toggle button is a bit hard to align so I used relative positioning. 

Tim
It sill looks a bit off http://prntscr.com/il8pil

Basically what we would need is to align-items: baseline
I tested it quickly and there's an issue with the arrow we need to figure out, but aside from that, it should be good
Assignee: rpirritano → tim.xie
Plase ignore my previous comment, I didn't build with your patch … (need another coffee :D)
With you actual patch, this is looking good !

So before we can land this, could you do 2 things: 
 - squash your 2 commits together, there's no need for separate commits here
 - append "; r=nchevobbe." at the end of the first line of your commit message. That's something that should be done everytime so it ping the reviewer that they have a review to do.

After that, we'll push to TRY to run all the tests of the repo, and if everything is okay, we'll land the patch :)

Thanks !
Attachment #8954661 - Attachment is obsolete: true
Attachment #8954927 - Attachment is obsolete: true
I squash the commits together (I think), and pushed it to review. Please let me know if there is a problem. 

Thank you!
Tim
It does look good :)
I have one very last thing I should have noticed before: the XHR pill has an inline-start margin that we don't need anymore.

So in this rule: https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/themes/webconsole.css#1007-1013 , we should override this property: https://searchfox.org/mozilla-central/source/devtools/client/themes/webconsole.css#295 with `margin-inline-start: 0`.

Also, I am going to push the patch to TRY (our CI infra) so we can see if we need to update a test.
It seems like a few tests failed on TRY, but I have no idea what the results are showing....
I don't think they are related to your patch.
Most of the time TRY shows Intermittent failures (aka "Oranges"), which are the result of racy tests / features.
Attachment #8955613 - Attachment is obsolete: true
Attachment #8955613 - Flags: review?(nchevobbe)
I just updated the margin-inline-start property mentioned above and pushed to review.
Comment on attachment 8957473 [details]
Bug 1415211 - Realigned XHR label, set property margin-inline-start of XHR to 0;

https://reviewboard.mozilla.org/r/226376/#review232268

That's perfect Tim,thanks a lot !
I pushed to TRY one more time to be extra careful, but when the jobs are over I think we'll be able to land this
Attachment #8957473 - Flags: review?(nchevobbe) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bf3a552fc51
Realigned XHR label, set property margin-inline-start of XHR to 0; r=nchevobbe
Thank you very much!
https://hg.mozilla.org/mozilla-central/rev/6bf3a552fc51
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
I have reproduced this bug with Nightly 58.0a1 (2017-11-07) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID  : 20180322152034
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [bugday-20180321]
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: