Closed Bug 895429 Opened 7 years ago Closed 6 years ago

Change nooverflow attribute to overflows

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: aks)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css])

Attachments

(1 file, 3 obsolete files)

"nooverflow" is a negative attribute, and it's also awkward with those two o's in there.

Mike de Boer suggestion changing this attribute to "overflows" (which defaults to true if not present).
To fix this bug, you'll need to clone the repository at https://hg.mozilla.org/projects/ux/.

Search for "nooverflow" in the /browser directory to see where it is referenced.
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P5][good-first-bug][mentor=jaws][lang=js][lang=css]
Whiteboard: [Australis:M?][Australis:P5][good-first-bug][mentor=jaws][lang=js][lang=css] → [Australis:M?][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css]
I can help here !
Assignee: nobody → aksht.kedia
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Version: unspecified → Trunk
Attached patch 895429.patch (obsolete) — Splinter Review
Hope this fixes stuff. :)
Attachment #795062 - Flags: review?(jaws)
Comment on attachment 795062 [details] [diff] [review]
895429.patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2280,5 @@
>  
>      while(child && this._target.clientWidth < this._target.scrollWidth) {
>        let prevChild = child.previousSibling;
>  
> +      if (!child.hasAttribute("overflows")) {

Please change this to:
if (child.getAttribute("overflows") != "false") {
since the overflows attribute is optional and the behavior should default to items being overflowed.
Attachment #795062 - Flags: review?(jaws) → feedback+
Attached patch 895429.patch (obsolete) — Splinter Review
Made the required the changes. This is my first bug and the first patch I uploaded was when I attened a Mozilla BootCamp so there were people there to help me but now back home I am having a bit of problems on with the hg commands and the procedure. Hope this patch is works else please help me !
Attachment #795062 - Attachment is obsolete: true
Attachment #796615 - Flags: review?(jaws)
The new patch doesn't contain any differences from the previous patch.

If you are using patch queues, after you make your changes run `hg qref` to refresh the patch. Then upload the new patch.

If you are not using patch queues, then make sure that your changes are saved before running `hg diff > patch.txt`.
Akshat, if you need any help with using hg or creating patches, you can always ask in #introduction on IRC (irc.mozilla.org).

It's a help channel for new contributors, and there are usually people there who can advise and answer questions. :)
Any update on this? Let me know if there is anything I can do to help.
Flags: needinfo?(aksht.kedia)
Attached patch 895429.patch (obsolete) — Splinter Review
Okay this time I did it completely on my own and finally learned how to make a patch ! :) Lets hope this patch is valid. If not please let me know where am I going wrong.
Attachment #796615 - Attachment is obsolete: true
Attachment #808158 - Flags: review?(jaws)
Flags: needinfo?(aksht.kedia)
Comment on attachment 808158 [details] [diff] [review]
895429.patch

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1629,5 @@
>          widget[prop] = aData[prop];
>        }
>      }
>  
> +    const kOptBoolProps = ["removable", "showInPrivateBrowsing", "overflows"]

nit: Please add a semicolon to the end of this line.

@@ +2370,3 @@
>          this._collapsed.set(child.id, this._target.clientWidth);
>          child.classList.add("overflowedItem");
> +        child.setAttribute("customizableui-anchorid", this._onClickChevronron.id);

This looks broken, this._onClickChevronron doesn't exist.
Attachment #808158 - Flags: review?(jaws) → feedback+
Attached patch 895429.patchSplinter Review
Made the above changes.
Attachment #808158 - Attachment is obsolete: true
Attachment #809144 - Flags: review?(jaws)
Comment on attachment 809144 [details] [diff] [review]
895429.patch

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

Thanks! These changes look good, and they passed all the customizableui tests on my local machine. I'll push it to the UX branch now.
Attachment #809144 - Flags: review?(jaws) → review+
Whiteboard: [Australis:M?][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css] → [Australis:M9][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css][fixed-in-ux]
Woohoo ! :D Thanks a ton for being with me on this the whole time :)
Can you please vouch for my Mozillians profile. Here's the link https://mozillians.org/en-US/u/aksht.kedia/ Thank You !
https://hg.mozilla.org/mozilla-central/rev/07c558e72010
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css][fixed-in-ux] → [Australis:M9][Australis:P5][good first bug][mentor=jaws][lang=js][lang=css]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.