Closed Bug 1106010 Opened 9 years ago Closed 9 years ago

"Beta" tag overlaps the Loop panel heading

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
2

Tracking

(firefox35+ verified, firefox36+ verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox35 + verified
firefox36 + verified
firefox37 --- verified
backlog Fx35+

People

(Reporter: Gijs, Assigned: mikedeboer)

References

Details

(Whiteboard: [rooms])

Attachments

(3 files)

Attached image Screenshot
The "beta" banner overlaps the text ("no current notifications"), and there is random whitespace at the bottom. This is on current nightly (Nov. 27).
Assignee: nobody → mdeboer
Severity: normal → blocker
Status: NEW → ASSIGNED
Iteration: --- → 37.1
Points: --- → 2
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Whiteboard: [rooms]
Summary: Hello panel looks broken on OS X → Hello panel looks broken with an empty rooms list when connected as Guest
fwiw, re the summary change, it looks just as broken before clicking the broken 'get started' link...
Flags: needinfo?(mreavy)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Status: REOPENED → ASSIGNED
[Tracking Requested - why for this release]:
We need this fix for "Rooms", which is already enabled in Fx35.
backlog: --- → Fx35+
Hi Mike,  Is this a bug you're able to do this week so we can uplift into Beta Monday for testing?
Flags: needinfo?(mdeboer)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Flags: needinfo?(mdeboer)
Resolution: --- → DUPLICATE
Matt -- Given the original report, why is this a dup of bug 1089191?

Original report:
The "beta" banner overlaps the text ("no current notifications"), and there is random whitespace at the bottom. This is on current nightly (Nov. 27).
Flags: needinfo?(MattN+bmo)
Gijs told me about this bug in-person and mentioned "random whitespace at the bottom" which is what 1089191 is about. This bug ended up talking about three different issues. I'll re-open for the Beta tag overlap.
Blocks: 1076709
Status: RESOLVED → REOPENED
Flags: needinfo?(MattN+bmo) → needinfo?(mdeboer)
Resolution: DUPLICATE → ---
Summary: Hello panel looks broken with an empty rooms list when connected as Guest → "Beta" tag overlaps the Loop panel heading
Flags: qe-verify?
Flags: firefox-backlog+
Are we still keeping the Beta tag for Fx36? When can we drop the Beta tag?
I feel we should keep the Beta tag until we're confident that the product is solid and polished.  I don't think we can tie that to a release but to success criteria for the product.  I want to talk more this week about that.
Status: REOPENED → ASSIGNED
Iteration: 37.1 → 37.2
Flags: qe-verify? → qe-verify+
Comment on attachment 8533709 [details] [diff] [review]
Patch v1: make sure enough UI elements are always visible when not signed in to FxA

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

Looks better! Though my journey through this part of the Panel code made me cringe a little :) (I think [tech-debt] bugs for proper refactoring are already filed anyway)

::: browser/components/loop/content/js/panel.jsx
@@ +928,5 @@
>        }
>  
> +      // Determine which buttons to NOT show.
> +      var hideButtons = [];
> +      if (!this.state.userProfile && !this.props.showTabButtons)

Side note: I was intrigued and checked what this `showTabButtons` prop was used for; it seems to be only used in panel_test.js to "force rendering" the buttons in this context… Boy, this part needs refactor. At least we should comment about it in the appropriate propTypes section.

@@ +929,5 @@
>  
> +      // Determine which buttons to NOT show.
> +      var hideButtons = [];
> +      if (!this.state.userProfile && !this.props.showTabButtons)
> +        hideButtons.push("contacts");

Nit: I think we've been avoiding omitting curly brackets for single line statements so far.

::: browser/components/loop/content/shared/css/panel.css
@@ +192,5 @@
>  }
>  
>  .room-list {
>    max-height: 335px; /* XXX better computation needed */
> +  min-height: 80px;  /* Show at least the space for one element */

It's matter of taste really, but my personal tests indicated 7px as a better value — see upcoming screenshot.
Attachment #8533709 - Flags: review?(nperriault) → review+
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #20)
> Created attachment 8533987 [details]
> Suggested css modifications

OK, you win :) Thanks for the review!
Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/9386c1c866cf

I will file a follow-up bug to fix the SocialAPI panel frame sizing code to allow for a different min-height/ min-width.
(In reply to Mike de Boer [:mikedeboer] from comment #22)
> I will file a follow-up bug to fix the SocialAPI panel frame sizing code to
> allow for a different min-height/ min-width.

Bug 1089191 could handle this
https://hg.mozilla.org/mozilla-central/rev/9386c1c866cf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
The blue icon looks to me a little odd in the middle of the panel http://i.imgur.com/P23gCMz.png
Also, any idea why the conversation list got unsorted ?
(In reply to Paul Silaghi, QA [:pauly] from comment #25)
> The blue icon looks to me a little odd in the middle of the panel
> http://i.imgur.com/P23gCMz.png

Well, we had no UX spec for this situation and we're just engineers :p NI :sevaan here.

> Also, any idea why the conversation list got unsorted ?

This is by design, they're sorted by latest activity ctime desc.
Flags: needinfo?(sfranks)
Anyway, marking verified since the initial problem is fixed.
37.0a1 (2014-12-11) Win 7
Status: RESOLVED → VERIFIED
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #26)
> (In reply to Paul Silaghi, QA [:pauly] from comment #25)
> > The blue icon looks to me a little odd in the middle of the panel
> > http://i.imgur.com/P23gCMz.png
> 
> Well, we had no UX spec for this situation and we're just engineers :p NI
> :sevaan here.

I would rather that row just not be displayed unless the user is logged in.
Flags: needinfo?(sfranks)
Comment on attachment 8533709 [details] [diff] [review]
Patch v1: make sure enough UI elements are always visible when not signed in to FxA

Approval Request Comment
[Feature/regressing bug #]: beta tag

[User impact if declined]: Beta tag can partly cover UI elements when a Guest (which is the default)

[Describe test coverage new/current, TBPL]: on m-c for several days; manually checked.

[Risks and why]: low risk - adjusts positioning and tweaks logic for button hiding.

[String/UUID change made/needed]: none
Attachment #8533709 - Flags: approval-mozilla-beta?
Attachment #8533709 - Flags: approval-mozilla-aurora?
Attachment #8533709 - Flags: approval-mozilla-beta?
Attachment #8533709 - Flags: approval-mozilla-beta+
Attachment #8533709 - Flags: approval-mozilla-aurora?
Attachment #8533709 - Flags: approval-mozilla-aurora+
Verified fixed 36.0a2 (2014-12-17), 35b4 Win 7
You need to log in before you can comment on or make changes to this bug.