Closed Bug 1088780 Opened 7 years ago Closed 6 years ago

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

Categories

(DevTools Graveyard :: WebIDE, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: sole, Assigned: yashmehrotra95, Mentored)

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(5 files, 4 obsolete files)

It's pretty bad UI to have to scroll here :)
Mentor: jryans
Whiteboard: [good first bug][lang=css]
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!
I also would like to contribute to this bug if possible what file needs to be changed? thanks Pete
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
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.
(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.
Now, we can view 40 characters at a time. Is it good or should I make it even bigger ?
Attachment #8588408 - Flags: review?(jryans)
Attached image Mozilla.png (obsolete) —
An example of URL after the patch is applied.
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
Attachment #8588408 - Flags: review?(jryans)
Attached patch 1088780.diff (obsolete) — Splinter Review
Upto 40 characters can be viewed in the url field. Also integrated flex as suggested. Indentation - 2 spaces.
Attachment #8588408 - Attachment is obsolete: true
Attachment #8591022 - Flags: review?(jryans)
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.
Attachment #8591022 - Flags: review?(jryans)
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.
Flags: needinfo?(jryans)
(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?
Flags: needinfo?(jryans)
Assignee: nobody → yashmehrotra95
Status: NEW → ASSIGNED
Attached patch 1088780.diff (obsolete) — Splinter Review
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.
Attachment #8588409 - Attachment is obsolete: true
Attachment #8591022 - Attachment is obsolete: true
Attachment #8596133 - Flags: review?(jryans)
Full size screen-shot.
Mid Size Screenshot.
Small Size Screenshot.
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.
Attachment #8596133 - Flags: review?(jryans) → feedback+
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.
(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.
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.
(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.
Attached patch 1088780.diffSplinter Review
changed class name.
Attachment #8596133 - Attachment is obsolete: true
Attachment #8596704 - Flags: review?(jryans)
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
Attachment #8596704 - Flags: review?(jryans) → review+
Your welcome, and thanks a lot for your help.
https://hg.mozilla.org/integration/fx-team/rev/40880379b05f
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/40880379b05f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.