Closed
Bug 1193666
Opened 9 years ago
Closed 9 years ago
Context Tile/Room Name surround colour needs changing
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox42 affected, firefox43 verified)
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Whiteboard: [visual refresh])
Attachments
(3 files, 2 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
sevaan
:
ui-review+
|
Details |
(deleted),
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
The surround colour for the room name / context tile needs changing for the standalone + conversation window - currently its conflicting with that of the hover for the context.
Assignee | ||
Comment 1•9 years ago
|
||
This fixes the background colour, and it fixes the fallback icon that we use on desktop - when I'd implemented the media views, I forgot to change the useDesktopPaths item to use the passed in props.
I've also removed passing useDesktopPaths to TextChatRoomName as it didn't need it.
Attachment #8646890 -
Flags: review?(andrei.br92)
Assignee | ||
Comment 2•9 years ago
|
||
Screenshot with left-to-right as follows:
- Current display
- Fixed surround colour and icon
- Fixed with Hover effect showing.
I'm just concentrating on the background & icon in this bug as we'll need to uplift these to 42.
Attachment #8646891 -
Flags: ui-review?(vpg)
Comment 3•9 years ago
|
||
Comment on attachment 8646891 [details]
Screenshot of patch effects
I wouldn't have hover feedback on 2 elements, we either have the link underlining hover or the whole thing. I would go for only the link but let's ask Sevaan as he might think differently.
THanks
Flags: needinfo?(sfranks)
Updated•9 years ago
|
Attachment #8646891 -
Flags: ui-review?(vpg) → ui-review?(sfranks)
Comment 4•9 years ago
|
||
Comment on attachment 8646891 [details]
Screenshot of patch effects
I like the tile colour background change on hover because it's such great visual feedback, and I think it's okay that the link underlines at the same time...because it's not two elements, really...we need to think of the tile as one whole element.
However, in this case the background colour does conflict with the background of the context area and it looks off.
In the future this won't matter, because the entire context area may be removed completely since any associated context will just be opened up as a tab in web sharing.
Until then though, I don't think a background change of the tile is necessary and we could just stick with the link underline on hover.
Vicky, what do you think about changing the border colour of the tile on hover if we leave the background white?
Flags: needinfo?(sfranks) → needinfo?(vpg)
Attachment #8646891 -
Flags: ui-review?(sfranks) → ui-review-
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8646890 [details] [diff] [review]
Fix surrounding colour of context tiles for Loop's text chat views. Also fix the fallback icon for context on desktop.
Will update tomorrow with responses from UX.
Attachment #8646890 -
Flags: review?(andrei.br92)
Comment 6•9 years ago
|
||
(In reply to Sevaan Franks [:sevaan] from comment #4)
> Comment on attachment 8646891 [details]
> Screenshot of patch effects
>
> I like the tile colour background change on hover because it's such great
> visual feedback, and I think it's okay that the link underlines at the same
> time...because it's not two elements, really...we need to think of the tile
> as one whole element.
>
> However, in this case the background colour does conflict with the
> background of the context area and it looks off.
>
> In the future this won't matter, because the entire context area may be
> removed completely since any associated context will just be opened up as a
> tab in web sharing.
>
> Until then though, I don't think a background change of the tile is
> necessary and we could just stick with the link underline on hover.
>
> Vicky, what do you think about changing the border colour of the tile on
> hover if we leave the background white?
Sevaan, that could be a fair solution, though, I am not sure how strong feedback will that be... 1px line, should change color drastically, right? I thought about the light blue competing there, but since hover is just a moment (and hopefully the hovering animation will be implemented soon), I don't see it unviable.
Flags: needinfo?(vpg)
Updated•9 years ago
|
Rank: 17
Flags: qe-verify+
Whiteboard: [visual refresh]
Assignee | ||
Comment 7•9 years ago
|
||
Sevaan, any suggestion on the way forward here?
Flags: needinfo?(sfranks)
Comment 8•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #7)
> Sevaan, any suggestion on the way forward here?
Mock attached.
On hover let's keep the URL underline, change the board colour to the Hello Active Blue (#5CCCEE) and increase the border thickness by 2px.
Flags: needinfo?(sfranks)
Assignee | ||
Comment 9•9 years ago
|
||
Sevaan, how's this. Non-hover on the left, hover versions on the right.
Attachment #8646891 -
Attachment is obsolete: true
Attachment #8652323 -
Flags: ui-review?(sfranks)
Assignee | ||
Comment 10•9 years ago
|
||
Updated for ux-feedback, changes the hover settings and fixes the fallback icon that's used on desktop.
I've also removed passing useDesktopPaths to TextChatRoomName as it didn't need it.
Attachment #8646890 -
Attachment is obsolete: true
Attachment #8652324 -
Flags: review?(mdeboer)
Updated•9 years ago
|
Attachment #8652323 -
Flags: ui-review?(sfranks) → ui-review+
Comment 11•9 years ago
|
||
Comment on attachment 8652324 [details] [diff] [review]
Fix surrounding colour of context tiles for Loop's text chat views. Also fix the fallback icon for context on desktop.
Review of attachment 8652324 [details] [diff] [review]:
-----------------------------------------------------------------
Nice! Thanks :)
::: browser/components/loop/content/shared/css/common.css
@@ +586,5 @@
> }
>
> .clicks-allowed.context-wrapper:hover {
> + border: 2px solid #5cccee;
> + padding: calc(.8em - 1px);
Well, there's a good explanation for this, but could you please document this above in a comment for future eyes?
Attachment #8652324 -
Flags: review?(mdeboer) → review+
Comment 13•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 14•9 years ago
|
||
Verified the changes on Firefox Developer Edition 43.0a2 across platforms (Windows 7 64-bit, Windows 10 64bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit) and no issues were found.
You need to log in
before you can comment on or make changes to this bug.
Description
•