Closed Bug 908241 Opened 6 years ago Closed 6 years ago

[SMS] Add attachment highlight state doesn't extend to the top edge

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, b2g-v1.1hd ?)

RESOLVED INVALID
blocking-b2g -
Tracking Status
b2g-v1.1hd --- ?

People

(Reporter: amylee, Assigned: pivanov)

Details

(Whiteboard: sverd vsd, HD)

Attachments

(6 files, 2 obsolete files)

Hi, 

The blue highlight state doesn't extend to the top edge of the button.

Thanks
Amy, is this happening on HD only?
Flags: needinfo?(amylee.design)
(In reply to Wayne Chang [:wchang] from comment #1)
> Amy, is this happening on HD only?

Hi, 

This is HD and v1.1. What flag should this be changed to?
Component: Gaia::E-Mail → Gaia::SMS
Flags: needinfo?(amylee.design)
Summary: [Email] Add attachment highlight state doesn't extend to the top edge → [SMS] Add attachment highlight state doesn't extend to the top edge
Hey Pavel, can you check the highlight state for this?  The height probably just needs to be increased. Thanks!
Flags: needinfo?(pivanov)
(In reply to Amy from comment #2)
> (In reply to Wayne Chang [:wchang] from comment #1)
> > Amy, is this happening on HD only?
> 
> Hi, 
> 
> This is HD and v1.1. What flag should this be changed to?

Hey Wayne, just wanted to make sure you saw Amy's response :)
Flags: needinfo?(wchang)
Be careful as the CSS in that part is not so easy. :)

Could have regressed because of bug 902390.
I'd recommend we leave this for 1.1 and 1.1hd altogether, have it corrected on master/1.2 ?
:)
blocking-b2g: hd? → ---
Flags: needinfo?(wchang)
blocking-b2g: --- → koi?
(In reply to Wayne Chang [:wchang] from comment #6)
> I'd recommend we leave this for 1.1 and 1.1hd altogether, have it corrected
> on master/1.2 ?
> :)

Yes, makes sense to me.  Thanks Wayne!  Change the whiteboard tags to reflect this.
Whiteboard: helix vsd, HD → sverd vsd, HD
Attached file patch for Gaia/master (obsolete) —
Attachment #805965 - Flags: review?(sjochimek)
Flags: needinfo?(pivanov)
Comment on attachment 805965 [details]
patch for Gaia/master

I am not sure about the js part (apps/sms/js/thread_ui.js)
Redirect the review to a peer.
Attachment #805965 - Flags: review?(sjochimek) → review?(schung)
Attached image 2013-09-24-01-45-02.png (obsolete) —
Hi Amy, the patch pivanov provided will move the button to the top. Is that the result you desired for? Thanks.
Attachment #808692 - Flags: feedback?(amylee.design)
Steve, Victoria should be our only Visual Design contact for the Messages app.
Hi Steve, 

Yes if you want to have Victoria confirm that would be great. The original bug was filed because the highlight state (blue overlay) wasn't reaching the top edge of the button. This isn't shown in the screenshot you provided so I can't provide feedback.
Attached image sample.png
Hi Victoria/Amy, this patch moves both buttons to the top of the container for keep the highlight background position. Is this result fit to your design? Thanks.
Attachment #808692 - Attachment is obsolete: true
Attachment #808692 - Flags: feedback?(amylee.design)
Attachment #808965 - Flags: feedback?(vpg)
Attachment #808965 - Flags: feedback?(amylee.design)
Hi Steve, 

I'm happy with this. Victoria are you okay with this?
Flags: needinfo?(vpg)
I'm quite sure Victoria won't like it, as she already rejected a similar proposal I made ;)
The bug was filed for alignment issues of the highlight state and not for design revisions. Any design related feedback please consult with Victoria. Thanks
The alignment issue of the highlight state is due to fact that the button is not aligned to the top anymore. The cause is bug 902390 which increased the height by one pixel, but the real cause was the change to the new font which introduces that pixel as soon as we were typing one character. Bug 902390 was made to make that 1px change persistant, so that we don't have a reflow when we type the first character.

Victoria, maybe bug 902390 was a wrong solution to a good problem, and we should have decreased the font-size or the line-height instead ? Or maybe the final font has not the same height difference with the previous font ?
triage: not blocking v1.2, minus
blocking-b2g: koi? → -
Thanks Amy for bringing this up, this looks OK to me. But one question, is this a problem commming from the Building Block design or was it a mistake made in App? I'll needinfo Arnau to take a look at this in the original BB.

Thanks!
Victoria

(In reply to Amy from comment #14)
> Hi Steve, 
> 
> I'm happy with this. Victoria are you okay with this?
Flags: needinfo?(vpg)
Flags: needinfo?(arnau)
Ayman, please have a look at attachment 808965 [details], as I thought you were opposed to this change.
Flags: needinfo?(aymanmaat)
Hey Julien, we're mixing issues here. Nobody agreed on this change, but why is it in this bug? Can you raise a new one?

Thanks!

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Ayman, please have a look at attachment 808965 [details], as I thought you
> were opposed to this change.
Victoria, this is not a standard BB input field, the CSS for this component relies too much in js, adding inline styles to fix positions.

I would propose using flex-box for this component.
Flags: needinfo?(arnau)
Isabel, there is a misunderstanding here: the attachment is exactly the result of the proposed patch in this bug.

Arnau, yes, good idea and completely agree, we didn't want to change the whole layout for 1.2 when it was a blocker, but could be a good idea to do this now that it's not a blocker anymore.
(In reply to Julien Wajsberg [:julienw] from comment #20)
> Ayman, please have a look at attachment 808965 [details], as I thought you
> were opposed to this change.

Thanks for flagging Julien.

Indeed I am oppose to this change. This is not the solution we are looking for. I have stated that the send CTA should not drift upwards as the message field extends. I have given clear direction as such, so we need an alternative solution.
Flags: needinfo?(aymanmaat)
To be clear. both the bottom edge of the attach CTA and the bottom edge of the Send CTA need to remain anchored to the top edge of the top edge of the keyboard and must not drift upwards as the message field expands.

ni? me if you require further input
OK, then it is treated as custom.

Thanks
(In reply to Arnau March from comment #22)
> Victoria, this is not a standard BB input field, the CSS for this component
> relies too much in js, adding inline styles to fix positions.
> 
> I would propose using flex-box for this component.
Hey, I htink there's a big misunderstangding here, and what Amy is proposing to fix is just an offset higlight shape and not talking about any other solution. PLease, if you have a different bug to discuss open a new one, and treat it there. Let this one be fixed.

Thanks

(In reply to ayman maat :maat from comment #24)
> (In reply to Julien Wajsberg [:julienw] from comment #20)
> > Ayman, please have a look at attachment 808965 [details], as I thought you
> > were opposed to this change.
> 
> Thanks for flagging Julien.
> 
> Indeed I am oppose to this change. This is not the solution we are looking
> for. I have stated that the send CTA should not drift upwards as the message
> field extends. I have given clear direction as such, so we need an
> alternative solution.
No, this is not what should happen here, the fix is just to fix this (https://bug908241.bugzilla.mozilla.org/attachment.cgi?id=794082) gap between the button shape/gradient ant the top divider for that field. Theres a white gap there that is more evident when the higlight color is on, just a gap. And this: https://bug908241.bugzilla.mozilla.org/attachment.cgi?id=808965 should not happen at all.

Thanks!

(In reply to Steve Chung [:steveck](PTO 10/1~10/8) from comment #10)
> Created attachment 808692 [details]
> 2013-09-24-01-45-02.png
> 
> Hi Amy, the patch pivanov provided will move the button to the top. Is that
> the result you desired for? Thanks.
Comment on attachment 808965 [details]
sample.png

This should not happen. The input elements should remain on the bottom part of the component while the input area expand upwards.
Attachment #808965 - Flags: feedback?(vpg)
Attachment #808965 - Flags: feedback?(amylee.design)
Attachment #808965 - Flags: feedback-
Victoria> I also agree with what Amy asked to do here, but since everybody said "yeah this patch is ok" while it was not, I had to raise a flag here ;)
Hi Julien, 

Thanks for raising the flag. I was looking at the highlight state of the button and wasn't aware that changes were also made to layout of the text field.
Comment on attachment 805965 [details]
patch for Gaia/master

Hi pivanov, based on the Vicky's feedback, we still need to stick the button's position to the bottom. Please adjust the messages-input's margin and keep the buttonOffset logic in this patch, thanks.
Attachment #805965 - Flags: review?(schung)
I think this.INPUT_MARGIN is wrong and we need to check why.
Attached image After patch screenshot
Like this?
Attachment #815327 - Flags: feedback?(vpg)
Attachment #815327 - Flags: feedback?(amylee.design)
(In reply to Pavel Ivanov [:ivanovpavel] from comment #34)
> Created attachment 815327 [details]
> After patch screenshot
> 
> Like this?

The alignment of the buttons at the bottom look good to me. Has the highlight state been fixed so it reaches the top edge of the button? This was the original request. Thanks!
(In reply to Pavel Ivanov [:ivanovpavel] from comment #36)
> Created attachment 815382 [details]
> active state after patch
> 
> Sure :)

Looks great! Thanks Pavel
Hey Pavel, this solves the confusion around how the input behaves (expanding upwards while the buttons stay sticked to the bottom) I guess the highlight issue is just fixed, right? Also, did something happen to the divider line at the top, the one that separates the recipients area from the body? Because it looks super dark and it used to be in a lighter (medium) grey. If that's something you did not tpuch, i'll just file a different bug for it.

Thanks!
Attachment #815327 - Flags: feedback?(vpg) → feedback-
Hey Victoria,

the color of the divider is #4d4d4d. I think there are some difference because of screenshots ... but if you want to change something on it we can do this in different, because it not related to this one (just assign it to me :))
Comment on attachment 805965 [details]
patch for Gaia/master

Hi Victoria, please create another bug with color code if you feel the current divider color is wrong, thanks.

Hi Pavel, this css fixing with removing the unnecessary js logic is good. We should close with this patch and stress other UI issues in different bugs, thnaks.
Attachment #805965 - Flags: review+
Thanks Steve :)

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/400d5fec2f099397575ded3ef8544150a9b472c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Pavel Ivanov [:ivanovpavel] from comment #41)
> Thanks Steve :)
> 
> Landed on master:
> https://github.com/mozilla-b2g/gaia/commit/
> 400d5fec2f099397575ded3ef8544150a9b472c5

Hi Pavel, because this is landed on master will it make it to HD?  Or does it need to land on HD as well?
Flags: needinfo?(pivanov)
This won't go to hd if this is not hd+.

Right now this won't even go to 1.2.
This patch doesn't work with the french locale, the input zone goes below the "send" (now "Envoyer") button (see attachment).

There is a reason this part uses Javascript, you know. This is not easy.

We may be able to do this with flex-box, as we didn't have this tool back when we first implemented this.

Backing out for now, sorry...
reverted: c07e3f25f51392391d57f1018ea2409e5695ef53
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 805965 [details]
patch for Gaia/master

><html>
>  <head>
>    <meta http-equiv="Refresh" content="2; url=https://github.com/mozilla-b2g/gaia/pull/12267">
>  </head>
>  <body>
>    Redirect to pull request 12267
>  </body>
></html>
Attachment #805965 - Attachment is obsolete: true
Attached file patch for Gaia/master
oh putain ... :) can you review it? I made it with flexbox :)
Attachment #816726 - Flags: review?(felash)
Flags: needinfo?(pivanov)
(In reply to Julien Wajsberg [:julienw] from comment #44)
> This patch doesn't work with the french locale, the input zone goes below
> the "send" (now "Envoyer") button (see attachment).
> 
> There is a reason this part uses Javascript, you know. This is not easy.
> 
> We may be able to do this with flex-box, as we didn't have this tool back
> when we first implemented this.
> 
> Backing out for now, sorry...

Sorry I miss the locale issue... my bad :(
It remind me that we did apply flex-box for adjusting compose input size automatically long time ago, but it's removed since whole sturcture changed.
Hey Steve,
is my bad ... I miss that too ... but Thanks to Julien we catch it on time :) and I think it's ready for review again
Comment on attachment 816726 [details]
patch for Gaia/master

This regresses the "MMS" and the counter indicators (which are in the same elements iirc).

The easiest way to show it is to attach an image so that the "MMS" indicator is displayed.

Please request review again once you're ready !
Attachment #816726 - Flags: review?(felash)
+ comments on github !
Attachment #808965 - Flags: feedback-
Attachment #808965 - Flags: feedback-
Attachment #815327 - Flags: feedback?(amylee.design)
I think this is obsolete now, right?
Flags: needinfo?(amlee)
Yes I think so
Flags: needinfo?(amlee)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.