984140 - UITour: Info panels with no image are missing a left margin

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot of now left margin on an info panel

Since the spec only had panels with icons and buttons, I forgot to consider when those aren't provided. I've pushed the UITour catalogue to my gh-pages branch so it's easier to manually test other combinations in the future: http://mnoorenberghe.github.io/mozilla-uitour/demos/catalogue.html

Luckily I don't know of any plans to use such an info panel for 29 but we should probably fix this just in case.

Comment 1

[Matthew N. [:MattN]]

I'm not working on this at the moment but I think it would be valuable to fix for future tour info panels.

Comment 2

[Alex Gibson [:agibson]]

Hi Matt, just a friendly +1 to get this fixed at some point in the future. 

For the last iterations of various tours the copy/design folks expressed the wish to use info panels without an image, but we had to find something suitable to workaround this layout bug.

Comment 3

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

This bug will affect our design/copy for the Hello UITour.

I'm not adding as a blocker yet, but I would like to know if this is something we can land in the next week.

Thanks

Comment 4

[Matthew N. [:MattN]]

Removing needinfo from mreavy since this is a non-Hello change. I made it block the meta bug since it's wanted although not a hard blocker it seems.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Cory Price [:ckprice] from comment #3)
> This bug will affect our design/copy for the Hello UITour.

This isn't a blocker and comment 2 suggests there's already a workaround in place. "Nice to have" but we're not going to worry about it if it introduces complication.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Trivial fix. The negative margin on the icon container was shifting things over, so just hide the container instead of the icon itself.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Screenshot: fixed

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8537003 [details] [diff] [review]
Patch v.1

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

rs=me with the below fixed (assuming it works with and without the icon specified)

::: browser/modules/UITour.jsm
@@ +1168,5 @@
>  
>        tooltipTitle.textContent = aTitle || "";
>        tooltipDesc.textContent = aDescription || "";
>        tooltipIcon.src = aIconURL || "";
> +      tooltipContainer.hidden = !aIconURL;

I'm confused. Did you forget to qrefresh? You defined `tooltipIconContainer` but are referencing `tooltipContainer`?

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Yeah, dunno what happened there. Bad cut'n'paste when updating my tree I guess.

https://hg.mozilla.org/integration/fx-team/rev/889224b8a0fd

[Triage Comment]

Approving for aurora+beta since this is needed for the Hello tour in fx35 and is trivial and low-risk.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a51fc9b5868
https://hg.mozilla.org/releases/mozilla-beta/rev/6c84b644fb37

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/889224b8a0fd