Closed Bug 984140 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: MattN, Assigned: Dolske)

References

Details

(Whiteboard: [Australis:P4])

Attachments

(3 files, 1 obsolete file)

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.
I'm not working on this at the moment but I think it would be valuable to fix for future tour info panels.
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Flags: firefox-backlog+
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.
Points: --- → 2
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
Flags: needinfo?(mreavy)
Flags: needinfo?(dolske)
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.
Flags: needinfo?(mreavy)
(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.
No longer blocks: fx-UITour-Hello
Flags: needinfo?(dolske)
Attached patch Patch v.1 (obsolete) — Splinter Review
Trivial fix. The negative margin on the icon container was shifting things over, so just hide the container instead of the icon itself.
Assignee: nobody → dolske
Attachment #8537003 - Flags: review?(bmcbride)
Attached image Screenshot: fixed
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`?
Attachment #8537003 - Flags: review?(bmcbride) → review+
Attached patch Patch v.2Splinter Review
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.
Attachment #8537003 - Attachment is obsolete: true
Attachment #8537573 - Flags: approval-mozilla-beta+
Attachment #8537573 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/889224b8a0fd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Iteration: --- → 37.2
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.