1088780 - Input field for templates URL in preferences pane is just too small

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Soledad Penades [:sole] [:spenades]]

Attachment: how the input field looks

It's pretty bad UI to have to scroll here :)

Comment 1

[Wickie Lee [:Robocrypt]]

I would like to work on this bug. I am new to contributing to open source and am new to software development as a whole also. So if I get this then some guidance would be appreciated!

Comment 2

[Pete Chambers]

I also would like to contribute to this bug if possible what file needs to be changed? thanks Pete

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Sorry for the long delay!  It's best to set the "needinfo" flag below to "mentor" in the future to ensure questions aren't missed.

To get started with the code base, check out our hacking page[1].

This bug involves the "Preferences" screen in WebIDE:

1. Tools -> Web Developer -> WebIDE (Shift-F8)
2. Choose Project -> Preferences from the menu bar

The page displayed is "prefs.xhtml"[2].  It loads "deck.css"[3], which will probably be the right place to make a style change to resolve this bug.

I'll assign the bug to whoever posts a patch first.

Feel free to needinfo? or post a patch for feedback?, or ask questions in #devtools on IRC.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/prefs.xhtml
[3]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/themes/deck.css

Comment 4

[Yash Mehrotra]

Hi, I would like to work on this patch. I have set up my dev environment and will be submitting a patch in the next 2-3 days.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Yash Mehrotra from comment #4)
> Hi, I would like to work on this patch. I have set up my dev environment and
> will be submitting a patch in the next 2-3 days.

Okay, great!  If you have questions, be sure to use "needinfo?" below to ensure I don't miss them.

Comment 6

[Yash Mehrotra]

Attachment: Now upto 40 characters can be entered in the URL field.

Now, we can view 40 characters at a time. Is it good or should I make it even bigger ?

Comment 7

[Yash Mehrotra]

Attachment: Mozilla.png

An example of URL after the patch is applied.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8588408 [details] [diff] [review]
Now upto 40 characters can be entered in the URL field.

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

That's a good start!  But, I think it would be even better to make it flexible width that updates as the window changes.

Are you familiar with flexbox[1]?  This is a good place to use it, I think.  We have the label with a static width, but the input should grow to take the remaining space.

Roughly, you would set "display: flex" for the <label> that contains the <span> and <input>, so that flexbox mode is used to layout its child nodes.  Then, for the <input>, you would set "flex: 1" to have it grow to take up the available space.  Probably a few padding adjustments are needed to keep things looking nice.

Thanks for taking a look at this!

[1]: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Flexible_boxes

::: browser/devtools/webide/themes/deck.css
@@ +71,5 @@
>    display: inline-block;
>  }
> +
> +li > label > input[type="text"] {
> +    width: 240px;

Nit: For future changes, be sure to use 2 spaces for indentation.  See our style guide[1] for some other CSS style tips.  (Some files are not fully consistent, but we should try our best!)

[1]: https://wiki.mozilla.org/DevTools/CSSTips

Comment 9

[Yash Mehrotra]

Attachment: 1088780.diff

Upto 40 characters can be viewed in the url field. Also integrated flex as suggested. Indentation - 2 spaces.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8591022 [details] [diff] [review]
1088780.diff

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

Getting closer I think! :)

To speed up making changes, you can try the Style Editor in the Browser Toolbox[1] here.

1. Open WebIDE
2. Switch to Preferences pane
3. Open Browser Toolbox
4. Click the "window" / "pane" icon in the top right, choose "chrome://webide/content/prefs.xhtml"
5. Switch to Style Editor tab
6. Choose the deck.css file

Now, as you edit the file there, your changes are applied immediately.

[1]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

::: browser/devtools/webide/themes/deck.css
@@ +17,5 @@
>    padding: 20px;
>    background-image: linear-gradient(#fff, #ededed 100px);
>  }
>  
> +.flex > label {

Our CSS code style typically does not make a CSS class based around CSS feature, like "flex", so this feels a bit strange to me.

Instead, we tend to make a class to group elements that have a similar purpose.  So, the class name would be derived from the reason the elements are similar, not because of a CSS feature desired.  Then, in the stylesheet, that class is used to give them a consistent appearance.

I was hoping this was written down in a guide, but I did not find it. :(

So, a class name like "text-input" on the <li>s that contain text inputs seems reasonable here.

Also, in your current patch, these rules never match, because the ">" combinator requires label to be a direct child of .flex, but you've placed .flex on the <ul>.

Once you've changed the class name, I think it makes more sense to place it on the only <li> with a text input, so that would allow ">" to work.

@@ +79,5 @@
>    display: inline-block;
>  }
> +
> +li > label > input[type="text"] {
> +  width: 240px;

I should have said this explicitly last time: with flexbox, we don't need a hard coded width at all, since we want the width to grow with the size of the window.  So, let's remove this.

However, it's possible the <label> or <input> may need small margin tweaks, as flexbox alone seems to place them too close together.

Comment 11

[Yash Mehrotra]

Hi , sorry for the really late reply, my university exams were going on.

I changed and tested flex stuff and it works, but I had one doubt, how to increase the size of the input field without hardcoding the width. 
The labels under the flex are behaving responsively as we change the size of the window. I am just stuck at changing the input fields size without hardcoding it.
Thanks.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Yash Mehrotra from comment #11)
> Hi , sorry for the really late reply, my university exams were going on.
> 
> I changed and tested flex stuff and it works, but I had one doubt, how to
> increase the size of the input field without hardcoding the width. 
> The labels under the flex are behaving responsively as we change the size of
> the window. I am just stuck at changing the input fields size without
> hardcoding it.
> Thanks.

No worries!

I am not sure the label text really needs to change in width, I would think the key point is that the input itself should take up the space remaining after the label (with a little margin so the label doesn't run directly into the input).

By putting "display: flex" on the parent of the <input>, you're saying "the children of this element should use the flexbox display mode".  Then, by setting "flex: 1" on the <input> itself, you're saying (roughly) "this element should 1 unit of available flexible space from it's container".  If there were other siblings, they could be also get "flex: 1" to each divide the space equally, or you can use different numbers to divide the space in more complex ways.

Anyway, those 2 steps should be all you need for main point of the input's size responding.  (Of course, you'll use CSS to apply these settings as you've been doing so far.)

Does that clear it up?

Comment 13

[Yash Mehrotra]

Attachment: 1088780.diff

Hi, I did what you said. The parent of input contains the flex class and gave `flex:0.5` to input. I initially used `flex: 1` but it was looking a bit ugly. I also have attached screenshots with different panel sizes.
Thanks for your help. :)
Please tell me if any other changes/modifications are required.

Comment 14

[Yash Mehrotra]

Attachment: Screenshot from 2015-04-23 01:21:52.png

Full size screen-shot.

Comment 15

[Yash Mehrotra]

Attachment: Screenshot from 2015-04-23 01:22:03.png

Mid Size Screenshot.

Comment 16

[Yash Mehrotra]

Attachment: Screenshot from 2015-04-23 01:22:17.png

Small Size Screenshot.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8596133 [details] [diff] [review]
1088780.diff

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

Cool, I think it generally looks nice!

However, see my feedback in comment 10 about CSS class names.  A class like ".flex" doesn't fit our CSS style.

Comment 18

[Yash Mehrotra]

Hi, sorry , dont know how I missed that, can you suggest a good class name for this ? I tried searching the docs for it but couldn't find anything.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Yash Mehrotra from comment #18)
> Hi, sorry , dont know how I missed that, can you suggest a good class name
> for this ? I tried searching the docs for it but couldn't find anything.

I already did in comment 10.  Please re-read it.

Comment 20

[Yash Mehrotra]

Oh, my bad. I thought, since we are applying it to the label it wouldn't be good. Anyway, so I will be adding the class`text-input` to the label containing input.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Yash Mehrotra from comment #20)
> Oh, my bad. I thought, since we are applying it to the label it wouldn't be
> good. Anyway, so I will be adding the class`text-input` to the label
> containing input.

I think it's still okay in that case, yeah.  The key point is that the class should be about groups elements that are related in some way, but not just about a CSS feature to use.

Comment 22

[Yash Mehrotra]

Attachment: 1088780.diff

changed class name.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8596704 [details] [diff] [review]
1088780.diff

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

Great, thanks for working on this!

This should be fine to land, but I've pushed it to our try server (testing) just to be sure:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eb0e32970d6

Comment 24

[Yash Mehrotra]

Your welcome, and thanks a lot for your help.

Comment 25

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/40880379b05f

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40880379b05f