926385 - [Messages] saner button css rules

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

We now have a beautiful building block for buttons, let's use it instead of copying its rule.

Bug 905208 imports it in index.html, but most of it is overriden by what's in root.css and other rules.

In this bug I'd like to remove all the <button> rules from root.css, remove the custom rules for the for the mms download button (which makes it look way nicer !) and possibly others, and replace some "<a role='button'>" with real "<button>"s.

Comment 1

[kumar rishav (:rishav_)]

Hi juliew
I am interested to work on this.
Can I get assigned this bug? Also any info about this would be helpful. :)
Thanks

Comment 2

[Julien Wajsberg [:julienw]]

This is more for maintenance than for performance. I don't expect any perf improvement with this.

Comment 3

[Julien Wajsberg [:julienw]]

Hey Rishav, actually I already said all in comment 0.

Please tell me if you need any help.

Comment 4

[kumar rishav (:rishav_)]

Attachment: Bug 926385 - [Messages] saner button css rules

Hi juliew
I removed all unnecessary stuff from root.css.Have a look on this PR.
Is it ok?
Thanks

Comment 5

[Rick Waldron [:rwaldron]]

(In reply to kumar rishav from comment #4)
> Created attachment 8376195 [details] [review]
> Bug 926385 - [Messages] saner button css rules
> 
> Hi juliew
> I removed all unnecessary stuff from root.css.Have a look on this PR.
> Is it ok?
> Thanks

Perhaps this bug's title should be updated?

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8376195 [details] [review]
Bug 926385 - [Messages] saner button css rules

Please only remove the <button> styles, as removing everything leads to some regressions and we should try to do this in other bugs if necessary.

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Rick Waldron [:rwaldron] from comment #5)
> (In reply to kumar rishav from comment #4)
> > Created attachment 8376195 [details] [review]
> > Bug 926385 - [Messages] saner button css rules
> > 
> > Hi juliew
> > I removed all unnecessary stuff from root.css.Have a look on this PR.
> > Is it ok?
> > Thanks
> 
> Perhaps this bug's title should be updated?

Nope, as we won't do that ;)

Comment 8

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> (In reply to Rick Waldron [:rwaldron] from comment #5)
> > (In reply to kumar rishav from comment #4)
> > > Created attachment 8376195 [details] [review]
> > > Bug 926385 - [Messages] saner button css rules
> > > 
> > > Hi juliew
> > > I removed all unnecessary stuff from root.css.Have a look on this PR.
> > > Is it ok?
> > > Thanks
> > 
> > Perhaps this bug's title should be updated?
> 
> Nope, as we won't do that ;)

Sure we do, all the time. "saner button css" isn't what this bug or patch is about, it's about replacing an anchor element with a button element and removing a mountain of css rules. "clean up root.css", "replace anchor with button"...

Comment 9

[Julien Wajsberg [:julienw]]

I mean, we won't remove all of root.css, only the button rules... so the title is closer to what we'll finally be doing (even if the current patch is not like this).

Comment 10

[kumar rishav (:rishav_)]

Attachment: Bug 926385 - [Messages] saner button css rules

Hi julien
I have made the necessary changes as you suggested.Have a look on attached PR.
Thanks

Comment 11

[Julien Wajsberg [:julienw]]

Hey Kumar, just an heads up here; I won't probably have time to review this properly before my holidays next week. We have a big pressure to finish other work right now.

I'm coming back Match 24th and I hope we'll be less rushed at this time.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8382967 [details] [review]
Bug 926385 - [Messages] saner button css rules

Reviewed on github, thanks. I'd like some more changes if possible :)

I just want to stress that your work here is really important because it's the start of some work to make the css for the Messages app a little more consistent. So thanks a lot and sorry for the delay.

Comment 13

[kumar rishav (:rishav_)]

Attachment: Bug 926385 - [Messages] saner button css rules

Hi juliew
I have made necessary changes as per our discussion on github/irc.Have a look on PR.
Thanks

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8404857 [details] [review]
Bug 926385 - [Messages] saner button css rules

r=me, thanks !

https://github.com/mozilla-b2g/gaia/pull/18238 is a rebased and squashed version of your patch, I'd like to wait for Travis before merging, I'll do it myself.

Comment 15

[Julien Wajsberg [:julienw]]

I put a NI so that I don't forget to merge when it's ready.

Comment 16

[Julien Wajsberg [:julienw]]

master: 18182d722d3182341f125667e2cb1045ed6d01bc

Comment 17

[Borja Salguero [:borjasalguero]]

Reopening this one because it has a regression in the SMS/MMS Composer:
https://github.com/borjasalguero/gaia/commit/7a32e43325628809d12571d0ed6f3288dc0716db

STR:
- Open Messaging App
- Go to 'new message'
- Add subject
- Type on the 'subject' input field

Expected:
Counter shows 'MMS' label on top of the 'send' text

Currently:
This 'label' is not shown anymore. That's why the back out.

Julien, could you take a look? Thanks!

Comment 18

[Julien Wajsberg [:julienw]]

Ok, the send button needs a "overflow visible" because the counter is out of it.

I'm adding this and relanding.

Thanks Borja !

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: PR: fixed the counter

Hey Borja,
can you have a look to the updated patch? I simply added "overflow: visible" for the messages-send-button. Please tell me if you find anything else wrong :)

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8406831 [details] [review]
PR: fixed the counter

Hey Steve,

can you help to double check the fix before I reland?

Comment 21

[Steve Chung [:steveck]]

Attachment: Screen Shot 2014-04-04 at 18.18.06.png

Looks like the new button styling will remove the background color if button is in li. It seems not what we expected for the download button.

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8406831 [details] [review]
PR: fixed the counter

This patch also has a conflict with the css lister list, could you also update this? Thanks.

Comment 23

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #21)
> Created attachment 8410926 [details]
> Screen Shot 2014-04-04 at 18.18.06.png
> 
> Looks like the new button styling will remove the background color if button
> is in li. It seems not what we expected for the download button.

I thought it was nicer like this ;)

More seriously, we'll need to refresh it so I thought we would do the correct design at this moment. I'll still take a look :)

Comment 24

[Julien Wajsberg [:julienw]]

Steve, I rebased the patch, the merges were done automatically by git :)

I don't find the original spec for the dowload button but the new spec is bug 980461. We see we'll need to do specific code for this anyway so that's why I suggest that we don't spend time to do the old spec (that I don't find ;) ). But tell me if you want that I do it again.

Comment 25

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Steve, I rebased the patch, the merges were done automatically by git :)
> 
> I don't find the original spec for the dowload button but the new spec is
> bug 980461. We see we'll need to do specific code for this anyway so that's
> why I suggest that we don't spend time to do the old spec (that I don't find
> ;) ). But tell me if you want that I do it again.

Oh, thanks for finding the new spec! If Oleg is working on the refresh, I think it's fine to leave the button as it is.

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8406831 [details] [review]
PR: fixed the counter

Thanks again for the rebasing and fixing :)

Comment 27

[Julien Wajsberg [:julienw]]

Github didn't want to merge so I rebased again and waiting for a green travis. Thanks !

Comment 28

[Julien Wajsberg [:julienw]]

master: 596ddc045ba47bd9f5f0b072343e3e2b400cc715