Closed Bug 1506540 Opened Last year Closed 8 months ago

Conforming style tweaks to Edit and Resend (button states, header title behaviour)

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: mcroud, Assigned: solaocodes, Mentored, NeedInfo)

Details

(Keywords: good-first-bug, Whiteboard: good-first-bug)

Attachments

(11 files, 1 obsolete file)

Attached image edit-resend-nitpick.png
Three minor nit-picks I have noticed while using the newly designed Edit and Resend panel:

1. (see figure A in the image) In extreme narrow widths, the title in the header wraps. This behaviour seems unique to this panel, all other pane titles overflow off the right edge when there is no more room. 
Additionally, maybe the buttons become left aligned in this rather peculiar width?

2. (see figure B in the image) I had no idea there was such a method as "Options", so this is my error! Increasing the width of this input to make the word "Options" appear comfortably would be great.

3. As we are using Photon Micro buttons can we please also include the correct focus and hover states (colors etc) as written in the design docs here: https://design.firefox.com/photon/components/buttons.html#primary-3  

If a high res mockup would be appreciated for these amends for guidance, let me know and I can cook something up!
Thanks for the report Matt!

Honza
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: good-first-bug
I'd be glad to fix these issues!
Attached patch bug1506540.patch (obsolete) — Splinter Review
Here's a patch for all the changes, except for the button alignment at small widths of the panel.

@Matt: For the "Cancel" button hover states, in the annotations for the new design you gave me, the button background colors weren't default background colors as per Photon specifications. I gave the buttons Photon's default hover background color values, but I can change them if that's not correct.

I'm having some difficulties with the button alignment. Having them wrap as flexbox items seems to create difficulties aligning them horizontally, and many of my flexbox solutions make the buttons run off the page opposite to how the rest of the panel behaves (running off the page to the right, I can attach a screenshot if that's not a clear description). I'm thinking this is something that might have to be done in JavaScript, e.g. in https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/CustomRequestPanel.js, but I'm not sure. Any suggestions?
Attachment #9027084 - Flags: feedback?(odvarko)
Attachment #9027084 - Flags: feedback?(mcroud)
Attached image buttonstyle.png
Good spot! Yes the Cancel buttons are a yet-to-be-documented button style found in other parts of DevTools (Accessibility panel, Performance, Memory panel).
Look at the attached and use the styling for Dark/Light mode of these existing buttons for Cancel, thanks!
Comment on attachment 9027084 [details] [diff] [review]
bug1506540.patch

Review of attachment 9027084 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this!

Looks good to me in general, but maybe we could use an extra @media block for the buttons?

Something like:

@media (max-width: 200px) {
  .network-monitor .custom-request .custom-request-button-container {
    justify-content: flex-start;
  } 
}


... to make sure they are properly left aligned when the side-panel is narrow.

Honza
Attachment #9027084 - Flags: feedback?(odvarko) → feedback+
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #5)
> Comment on attachment 9027084 [details] [diff] [review]
> bug1506540.patch
> 
> Review of attachment 9027084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on this!
> 
> Looks good to me in general, but maybe we could use an extra @media block
> for the buttons?
> 
> Something like:
> 
> @media (max-width: 200px) {
>   .network-monitor .custom-request .custom-request-button-container {
>     justify-content: flex-start;
>   } 
> }
> 
> 
> ... to make sure they are properly left aligned when the side-panel is
> narrow.
> 
> Honza

Is there a way to make @media rules apply to specific html elements? I was under the impression @media rules only applied to print, screen, and speech. Otherwise, if my browser window is greater than my max-width, the buttons will still become unaligned if I make .custom-request-panel narrow. Or is a narrow browser width the only situation I need to be thinking about?
Flags: needinfo?(odvarko)
Sorry for jumping out of nowhere

> > Something like:
> > 
> > @media (max-width: 200px) {
> >   .network-monitor .custom-request .custom-request-button-container {
> >     justify-content: flex-start;
> >   } 
> > }
> > 
> > 
> > ... to make sure they are properly left aligned when the side-panel is
> > narrow.
> > 
> > Honza
> 
> Is there a way to make @media rules apply to specific html elements? I was
> under the impression @media rules only applied to print, screen, and speech.

Whatever is inside a media query will be applied to the elements you target with a selector.
The query (in this case) is against the viewport width (the devtools window)

> Otherwise, if my browser window is greater than my max-width, the buttons
> will still become unaligned if I make .custom-request-panel narrow. Or is a

This rule would apply only to narrow windows.

> narrow browser width the only situation I need to be thinking about?

@media (max-width: 200px)
Is basically saying: apply whatever is inside me unless the width exceeds 200px

So, for windows from 0 to 200px, the styles will be applied. Otherwise the entire block will be ignored.


@media (max-width: 200px) {           /* for windows smaller than 200px*/
  .custom-request-button-container {  /* apply to .custom-request-button-container */
     justify-content: flex-start;     /* this styles*/
  } 

  /* If you do so, you'll need to add a margin-left to the cancel button inside the mq */

  .custom-request-button-close {     /* target the close button*/  
     margin-left:4px;                /* and give it a margin */
  }
}

Hope this helps,
cheers
@Adam, is that clear now?

Honza
Flags: needinfo?(odvarko)
(In reply to Ariel Burone from comment #7)
> Sorry for jumping out of nowhere

No worries, thank you very much for the explanation :)

(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #8)
> @Adam, is that clear now?
> 
> Honza

Yes; I wasn't realizing that |chrome://devtools/content/netmonitor/index.html| could be targeted specifically with @media rules.

In the attached screenshot is a situation where the Edit and Resend panel can be made narrow enough to reproduce the alignment issue without changing the width of the netmonitor panel and triggering the @media rules. Any suggestions for resolving that?
Attached patch bug1506540.patchSplinter Review
Here's my fixes with the @media rules implemented. 

This still leaves the narrow width situation illustrated by my screenshot above. I've been thinking it could be fixed in one of the react files associated with the Network Details panel (https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/CustomRequestPanel.js, https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/MonitorPanel.js, https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/NetworkDetailsPanel.js), but I'm not as adept with react so I haven't gotten a fix together yet.
Attachment #9027084 - Attachment is obsolete: true
Attachment #9027084 - Flags: feedback?(mcroud)
Attachment #9029549 - Flags: feedback?(odvarko)
Comment on attachment 9029549 [details] [diff] [review]
bug1506540.patch

Review of attachment 9029549 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

Looks good to me in general (see couple of comments I made), but it would be great to have some feedback from Ariel.

Honza

::: devtools/client/netmonitor/src/assets/styles/CustomRequestPanel.css
@@ +65,5 @@
>  
> +  .network-monitor .custom-request .custom-request-button-container {
> +    display: flex;
> +    flex-direction: row;
> +    flex-wrap: wrap-reverse;

nit: Wrong indentation

::: devtools/client/netmonitor/src/assets/styles/NetworkDetailsPanel.css
@@ +392,5 @@
>  /* Custom request panel */
>  
> +@media (max-width: 130px) {
> +  .network-monitor .custom-request .custom-request-button-container {
> +    justify-content: flex-start;

when using `center` in case the width is smaller than 130px the buttons are nicely aligned.
(but it's a bit surprising that `left` doesn't work that way)
Attachment #9029549 - Flags: feedback?(odvarko) → feedback?(aburone)
Hi, sorry for the delay, missed the mail.

Not sure if this is what Honza expected, but I'll try my best :P
I can't apply the patch, I get a strange conflict, so I'm not really sure if I've got the correct stylesheets.
I'll attach 4 images and 2 css files, a patch would probably make things worse.

1st image is what I saw after applying manually Adam's changes. I see a 1px misalignment
2nd misaligned fixed
3rd right aligned buttons (not sure if the intention was getting them left or right aligned)
4th adds vertical space between the buttons when stacked

---

This is a "diff" created manually, sorry for the mess


file: netmonitor/src/assets/styles/CustomRequestPanel.css
.network-monitor .custom-request .custom-request-button-container {
  display: flex;
-  flex-direction:row; /* not necessary, that's the default */
  flex-wrap: wrap-reverse;
}


file: netmonitor/src/assets/styles/NetworkDetailsPanel.css
@media (max-width: 130px) {
  .network-monitor .custom-request .custom-request-button-container {
    justify-content: flex-start;
    padding-left: 16px;
  }

  .network-monitor .custom-request #custom-request-send-button {
-    margin-left: 0; /* fixes the alignment (off by 1px) */
  }
}


---

Not sure if the idea was to have the buttons left or right aligned

This is for right aligned buttons

file: netmonitor/src/assets/styles/NetworkDetailsPanel.css
@media (max-width: 130px) {
 - .network-monitor .custom-request #custom-request-send-button{
 -   margin-left:0;
 - }

  .network-monitor .custom-request .custom-request-button-container{
 -    justify-content:flex-start;
 -    padding-left:16px;
 +    justify-content:flex-end;
 +    margin-right:4px;
  }
}

file: netmonitor/src/assets/styles/CustomRequestPanel.css
@media (min-width: 130px) {
  .network-monitor .custom-request .custom-request-button-container {
    justify-content: flex-end;
 +    margin-right:4px;
  }

 - .network-monitor .custom-request #custom-request-send-button {
 -    margin-left:4px;
 -  }
}

.network-monitor .custom-request #custom-request-send-button {
 -  margin-right:16px;
 +  margin-left:4px;
}

-----
if you want some vertical spacing between the stacked buttons add this block 

@media (max-width: 130px) {
  .network-monitor .custom-request button {
    margin-bottom: 4px;
  }
}

/*
after this block
.network-monitor .custom-request button {}
*/



PS: not related to  this error. But the network panel shouldn't have a horizontal scrollbar when the timeline overflows to the right?
PS2: "Changes" tab can't be used directly as a diff, doesn't give the same contextual lines :.(
Attached image 1506540 1 initial
1px misalignment
Attached image 1506540 2 fixed margin
Misalignment fixed
Attached image 1506540 3 right aligned
Attachment #9029549 - Flags: feedback?(aburone) → feedback-
(In reply to Ariel Burone from comment #12)

Sorry for the delay myself. Thanks for the great feedback!

(In reply to Ariel Burone from comment #14)
> Created attachment 9030142 [details]
> 1506540 2 fixed margin
> 
> Misalignment fixed

Where/how did you fix the misalignment here? I implemented the changes from your CustomRequestPanel.css and NetworkDetailsPanel.css,  other than the changes for right-alignment with the buttons, and I'm still seeing misalignment with the buttons, even without implementing Honza's suggestion to change `justify-content` from `flex-start` to `center` at line 392 in NetworkDetailsPanel.css.


(In reply to Ariel Burone from comment #12)
> Not sure if the idea was to have the buttons left or right aligned

I'm not sure either. I left aligned them based on Matt's mock-up, https://bugzilla.mozilla.org/attachment.cgi?id=9024372.

Where is the code to fix the bug ? Can anyone help me ?

I would like to contribute to this issue, can someone assign the task to me.

Pls assign it to me, I would like to work on this

@Farheen: thanks for helping!

Please read carefully the thread here (there is a lot of useful info).
Especially comment #3, comment #5, comment #7

Related files are:

Honza

Assignee: nobody → farheennajeeb
Mentor: odvarko

Hi,

I'm an outreachy applicant. It's been a couple of days since last reply, in case this bug becomes available, I'd like to show my interest in working on it. Already read through the threads and understand the issue.

Thanks a ton!

Flags: needinfo?(odvarko)

@Farheen: are you working on this?
Honza

Flags: needinfo?(farheennajeeb)

@Sola: still interested in this bug?

Honza

Flags: needinfo?(odvarko) → needinfo?(solaocodes)

NI myself

Flags: needinfo?(odvarko)

Yes, I am

Flags: needinfo?(solaocodes)

Assigned to you, thanks!
Honza

Assignee: farheennajeeb → solaocodes
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)

@Sole: do you need any help?

Honza

Flags: needinfo?(solaocodes)

I've increased the width of the method input to fit "Options".

Having a bit of trouble getting the title in the edit & resend panel header to overflow instead of wrapping, I have a strong suspicion that it's an overflow/flexbox issue causing the problem. Still digging into it, but I'm open to any suggestions you have.

Haven't started on the buttons yet, but it seems I can mostly apply the changes @Ariel & @Adam made.

Flags: needinfo?(solaocodes)

Increase the minimum width of the custom method value to make all options appear comfortably.
Fix Custom Request Panel header title wrap bug.
Style & position Custom Requeset Panel buttons properly.

Hey Honza!

Just updated the Revision

Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e83e4d61954
Conforming style tweaks to Custom Request Panel in net monitor (button states, header title behaviour). r=Honza
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.