Closed Bug 1210563 Opened 4 years ago Closed 4 years ago

Links at the top can overlap title

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set

Tracking

(firefox44 affected, firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: wbamberg, Assigned: sidhus11, Mentored)

Details

(Whiteboard: [polish-backlog][difficulty=easy][good first bug][lang=css])

Attachments

(2 files, 3 obsolete files)

Attached image webide-bug.png
See attached screenshot.
Mentor: jryans
Whiteboard: [polish-backlog][difficulty=easy][good first bug][lang=css]
Hey, I'd like to try my hand at this bug. 
Could you point me to the file I shall be looking at for the code?
Solid question, Manav. Jryans?
Flags: needinfo?(jryans)
Ah, sorry!  Seems the original comment slipped through my deluge of bug mail.

To get started with the code base, take a look at our Hacking page[1].  That page is Mercurial-focused, but also has good general advice.  If you prefer to use Git, you can also check the new Contributing page[2].

To reach the screen from comment 0, you would:

1. Open WebIDE (Tools -> Web Developer -> WebIDE)
2. Go to the menu bar
3. Project menu -> Preferences

However, there are many pages that share the same styling, see also Project menu -> Manage Extra Components and others.

You can find the HTML for the preferences page[3] at devtools/client/webide/content/prefs.xhtml.  The CSS you will probably need to modify[4] is at devtools/client/webide/themes/deck.css.

If you have more questions, be sure to set the needinfo? flag at the bottom to "mentor" so I don't miss them.  If you come up with patch, also make to set either feedback (you'd like some advice) or review (you believe it's done) flags on the patch to my email.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://developer.mozilla.org/en-US/docs/Tools/Contributing
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/content/prefs.xhtml
[4]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/themes/deck.css
Flags: needinfo?(jryans)
Hi, can I be assigned to this bug? I'm new and am having trouble finding a first bug to work on that's not already taken!
Flags: needinfo?(jryans)
(In reply to Sunny Sidhu from comment #4)
> Hi, can I be assigned to this bug? I'm new and am having trouble finding a
> first bug to work on that's not already taken!

Our team usually waits for a first patch to be attached before assigning a bug.  Feel free to give it a try.
Flags: needinfo?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Our team usually waits for a first patch to be attached before assigning a
> bug.  Feel free to give it a try.

I've solved the problem, but I'm struggling to find the 'right' way to create a patch, I've read/seen 3 different methods. Could you point towards the right direction please?
Flags: needinfo?(jryans)
(In reply to Sunny Sidhu from comment #6)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > Our team usually waits for a first patch to be attached before assigning a
> > bug.  Feel free to give it a try.
> 
> I've solved the problem, but I'm struggling to find the 'right' way to
> create a patch, I've read/seen 3 different methods. Could you point towards
> the right direction please?

Hmm, there is a lot of different documentation, especially because there are both Git-based and Mercurial-based workflows.  And even within those, different approaches...

Anyway, I linked to both of those above in comment 3.  I'm not sure I know of other guides than those.

If you're not sure how to proceed, I'd recommend joining Mozilla IRC[1] on the #introduction channel where you can get some help in real time.

Or if that fails, try to describe to me what you've attempted, and we can try to work it out.

[1]: https://wiki.mozilla.org/Irc
Flags: needinfo?(jryans)
Attached patch bug1210563fix.patch (obsolete) — Splinter Review
Thanks, I was able to figure it out. Please find attached my patch.
Flags: needinfo?(jryans)
Attachment #8673286 - Flags: feedback+
Flags: needinfo?(jryans)
Comment on attachment 8673286 [details] [diff] [review]
bug1210563fix.patch

Changing to feedback request.  I'll take a look soon!
Attachment #8673286 - Flags: feedback+ → feedback?(jryans)
Attachment #8673286 - Attachment is obsolete: true
Attachment #8673286 - Flags: feedback?(jryans)
Attachment #8673291 - Flags: review?(jryans)
Comment on attachment 8673291 [details] [diff] [review]
rev1 - Alters code for positioning

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

Great, that takes care of the main issue!  See comments below about a few details.

Also, please add "r=jryans" to the end of the commit message for the next version.

::: devtools/client/webide/themes/deck.css
@@ +35,5 @@
>    margin-bottom: .5em;
>  }
>  
>  #controls {
> +  float: right;

Overall, that works nicely!

To keep the same position of the elements, we could also add "position: relative", and change top and right here to -10px, since the body has 20px padding.

Or, we could live with 10px extra space on the top and right around the controls.  What do you think is better?
Attachment #8673291 - Flags: review?(jryans) → feedback+
Assignee: nobody → sidhus11
Status: NEW → ASSIGNED
I think the method you mentioned would be better. Float works by removing the element from the normal document flow and then adjusts how other elements drift around it. Whereas the position is more 'explicit' and nicer, because you can move the element relative to where it should normally appear. The top and right properties also are positional properties, so they don't actually apply to the float method. It's probably also better to keep the controls positioned where they were initially intended to be.

Please find attached my revised patch.
Attachment #8673291 - Attachment is obsolete: true
Attachment #8675303 - Flags: review?(jryans)
Comment on attachment 8675303 [details] [diff] [review]
rev2 - Amends position property to relative for controls element

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

I should have mentioned this earlier on... I'm not sure how you've been testing changes so far, but there are two techniques available:

A. You can use the Browser Toolbox[1] to access other windows[2] like WebIDE
B. You can load WebIDE in a regular tab for inspection by going to chrome://webide/content/webide.xul in a tab
  * Some features of WebIDE may not work in this mode, but it's usually good enough for trying out CSS
  * You'll probably also need to change documents[2] here as well, since there are inner frames involved

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

::: devtools/client/webide/themes/deck.css
@@ +35,5 @@
>    margin-bottom: .5em;
>  }
>  
>  #controls {
> +  position: relative;

That's almost what I had in mind, but I meant these 3 lines *plus* the float: right you already had.  At the moment, the controls aren't right-justified anymore without the float: right.

Sorry for not being clear!

If you add that line back in as well, it should position the controls right were they were, but also they'll push down the header at small widths, like we're hoping for here.
Attachment #8675303 - Flags: review?(jryans)
Thanks for the information on testing changes! The Browser Toolbox makes life so much more easier.

Please find attached my revised my patch.
Attachment #8675303 - Attachment is obsolete: true
Attachment #8681858 - Flags: review?(jryans)
Comment on attachment 8681858 [details] [diff] [review]
rev 3 - Adds float property and amends position and margins

Great, this looks good to me!  Thanks for working on it.

I don't think there are any tests for this area, so I think we can land it directly.  I'll do that now.
Attachment #8681858 - Flags: review?(jryans) → review+
https://hg.mozilla.org/mozilla-central/rev/9e1f2504b081
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.