code review 6828044: go.talks/present: fix image size attributes

go.talks/present: fix image size attributes
The arg type for image sizes is int, using %s is wrong.

[francesc, 2012-11-07 02:24:41]

Hello adg@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.talks

[r, 2012-11-07 07:20:08]

If you're fixing a bug, say more. If you're changing the interface, don't.
Percent is better than pixels when presenting on a display of unknown size. It
looks like a bit of both is going on here.

[adg, 2012-11-07 07:47:35]

I don't think px is right, these are tag attributes, not css directives.

The change from %s to %v LGTM, though.


On 7 November 2012 03:24, <campoy@golang.org> wrote:

> Reviewers: adg,
>
> Message:
> Hello adg@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go.**talks <https://code.google.com/p/go.talks>
>
>
> Description:
> go.talks/present: Correctly handling image sizes.
> The arg type for image sizes is int, using %s is wrong.
>
> Please review this at
http://codereview.appspot.com/**6828044/<http://codereview.appspot.com/6828044/>
>
> Affected files:
>   M present/image.go
>
>
> Index: present/image.go
> ==============================**==============================**=======
> --- a/present/image.go
> +++ b/present/image.go
> @@ -39,7 +39,7 @@
>         case 0:
>                 // no size parameters
>         case 2:
> -               args = fmt.Sprintf("height=%s width=%s", arg[0], arg[1])
> +               args = fmt.Sprintf("height='%vpx' width='%vpx'", arg[0],
> arg[1])
>         default:
>                 return "", fmt.Errorf("incorrect image invocation: code %q
> %v", file, arg)
>         }
>
>
>

[francesc, 2012-11-07 16:47:16]

This is a really simple bug, I thought my description was clear enough (but
I tend to understand what I say...)

Andrew, I removed the px (in HTML5 the default unit for length is px) but
now I'm thinking, should I use %d instead of %v?


On Tue, Nov 6, 2012 at 11:47 PM, Andrew Gerrand <adg@golang.org> wrote:

> I don't think px is right, these are tag attributes, not css directives.
>
> The change from %s to %v LGTM, though.
>
>
> On 7 November 2012 03:24, <campoy@golang.org> wrote:
>
>> Reviewers: adg,
>>
>> Message:
>> Hello adg@golang.org (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go.**talks <https://code.google.com/p/go.talks>
>>
>>
>> Description:
>> go.talks/present: Correctly handling image sizes.
>> The arg type for image sizes is int, using %s is wrong.
>>
>> Please review this at
http://codereview.appspot.com/**6828044/<http://codereview.appspot.com/6828044/>
>>
>> Affected files:
>>   M present/image.go
>>
>>
>> Index: present/image.go
>> ==============================**==============================**=======
>> --- a/present/image.go
>> +++ b/present/image.go
>> @@ -39,7 +39,7 @@
>>         case 0:
>>                 // no size parameters
>>         case 2:
>> -               args = fmt.Sprintf("height=%s width=%s", arg[0], arg[1])
>> +               args = fmt.Sprintf("height='%vpx' width='%vpx'", arg[0],
>> arg[1])
>>         default:
>>                 return "", fmt.Errorf("incorrect image invocation: code
>> %q %v", file, arg)
>>         }
>>
>>
>>
>


-- 
--
Francesc

[francesc, 2012-11-07 16:48:28]

Hello adg@golang.org, r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-11-12 11:14:56]

LGTM

[adg, 2012-11-12 11:15:25]

Please rename teh CL description to

go.talks/present: fix image size attributes

[francesc, 2012-11-12 17:43:12]

*** Submitted as
http://code.google.com/p/go/source/detail?r=30052e7f1f3c&repo=talks ***

go.talks/present: fix image size attributes
The arg type for image sizes is int, using %s is wrong.

R=adg, r
CC=golang-dev
http://codereview.appspot.com/6828044