social buttons are too large

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 40
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch sdkbutton (obsolete) — Splinter Review
Attached patch fixes button sizing bug by adding sdk-button=true to the social buttons.  However, IMO it would be better to have a constrain-size=true attribute in case anyone decides to use sdk-button for something in the future (I am not certain whether there is some code out there looking for buttons with sdk-button=true).
Attachment #8591792 - Flags: feedback?(dao)
Comment on attachment 8591792 [details] [diff] [review]
sdkbutton

I'm not sure if this could cause trouble in the future. Probably best to get feedback from SDK people too.

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -1625,23 +1625,16 @@ richlistitem[type~="action"][actiontype=
> }
> .share-provider-button > .toolbarbutton-icon {
>   width: 16px;
>   min-height: 16px;
>   max-height: 16px;
> }
> 
> 
>-toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
>-  width: auto;
>-  height: auto;
>-  max-width: 32px;
>-  max-height: 24px;
>-}

Oh, yeah, thanks for removing this too.
Attachment #8591792 - Flags: feedback?(dao) → feedback+
Comment on attachment 8591792 [details] [diff] [review]
sdkbutton

Matteo, do you see any current or speculative problem with the social buttons using the sdk-button attribute?
Attachment #8591792 - Flags: feedback+ → feedback?(zer0)
The current use we have for "sdk-button" is actually only for the size, so I don't see any problem to use it atm, however I agreed that would be better having something like `constrain-size`, and then refactoring the SDK part too – I can take care of that.

However, it seems to me that we have a regression, since if I create a button via SDK, and I set a larger  image (e.g. 512x512), the button will be huge instead of be resized.
Attachment #8591792 - Flags: feedback?(zer0) → feedback+
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> The current use we have for "sdk-button" is actually only for the size, so I
> don't see any problem to use it atm, however I agreed that would be better
> having something like `constrain-size`, and then refactoring the SDK part
> too – I can take care of that.

Should we change sdk-button everywhere to constrain-size?  Or do you want both attributes?

> However, it seems to me that we have a regression, since if I create a
> button via SDK, and I set a larger  image (e.g. 512x512), the button will be
> huge instead of be resized.

I beleive that would be bug 1153243.
Flags: needinfo?(zer0)
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> > The current use we have for "sdk-button" is actually only for the size, so I
> > don't see any problem to use it atm, however I agreed that would be better
> > having something like `constrain-size`, and then refactoring the SDK part
> > too – I can take care of that.
> 
> Should we change sdk-button everywhere to constrain-size?  Or do you want
> both attributes?

I would say having just `constrain-size` would be better. Unless someone else wants to have a SDK specific attribute for some reason, I'm in favor of having just one generic attribute, like the one you suggested.
Flags: needinfo?(zer0)
Posted patch sdkbuttonSplinter Review
This changes sdk-button to constrain-size, and uses that for both sdk and social buttons.
Assignee: nobody → mixedpuppy
Attachment #8591792 - Attachment is obsolete: true
Attachment #8591888 - Flags: review?(dao)
Attachment #8591888 - Flags: feedback?(zer0)
Comment on attachment 8591888 [details] [diff] [review]
sdkbutton

Looks good to me, but the SDK changes needs a separate PR to http://github.com/mozilla/addon-sdk, that is main repo – any changes made directly to firefox source code will be overwritten in the next uplift, I'm afraid.
Attachment #8591888 - Flags: feedback?(zer0) → feedback+
Attachment #8591888 - Attachment is patch: true
Attachment #8591888 - Flags: review?(dao) → review+
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> Comment on attachment 8591888 [details] [diff] [review]
> sdkbutton
> 
> Looks good to me, but the SDK changes needs a separate PR to
> http://github.com/mozilla/addon-sdk, that is main repo – any changes made
> directly to firefox source code will be overwritten in the next uplift, I'm
> afraid.

Ok, I'll remove that part of the patch, can you submit that to the addon-sdk repo?
Flags: needinfo?(zer0)
Actually, I'll leave that part in for now, and addon-sdk can get the update hopefully prior to the next uplift.
https://hg.mozilla.org/mozilla-central/rev/cdebac12ce27
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Not sure if this change need a review Erik, but just to play safe. It's really a quick one. :)
Flags: needinfo?(zer0)
Attachment #8593967 - Flags: review?(evold)
Comment on attachment 8593967 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1936

We generally assume that if code has already been reviewed and landed in m-c then the uplift to github doesn't need a review.
Attachment #8593967 - Flags: review?(evold) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/dcd987d3a94af8a07d4a0f93296b7c2bcc737d14
Bug 1153952 - social buttons are too large

 - Updated the sdk specific attribute to the new more generic `constrain-size`, to have the buttons that fits the toolbar and the panel.

https://github.com/mozilla/addon-sdk/commit/6fca9e732d0b145e620ed03ffe46281c3f306b66
Merge pull request #1936 from ZER0/constrain-size/1153952

fix Bug 1153952 - social buttons are too large
You need to log in before you can comment on or make changes to this bug.