Closed Bug 926385 Opened 6 years ago Closed 6 years ago

[Messages] saner button css rules

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: rishav_)

References

Details

(Whiteboard: [mentor=:julienw][lang=css])

Attachments

(3 files, 2 obsolete files)

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.
Depends on: 905208
Hi juliew
I am interested to work on this.
Can I get assigned this bug? Also any info about this would be helpful. :)
Thanks
This is more for maintenance than for performance. I don't expect any perf improvement with this.
No longer blocks: messaging-perf
Hey Rishav, actually I already said all in comment 0.

Please tell me if you need any help.
Assignee: felash → rishav006
Whiteboard: [mentor=:julienw]
Hi juliew
I removed all unnecessary stuff from root.css.Have a look on this PR.
Is it ok?
Thanks
Attachment #8376195 - Flags: review?(felash)
(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 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.
Attachment #8376195 - Flags: review?(felash)
(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 ;)
(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"...
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).
Hi julien
I have made the necessary changes as you suggested.Have a look on attached PR.
Thanks
Attachment #8376195 - Attachment is obsolete: true
Attachment #8382967 - Flags: review?(felash)
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 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.
Attachment #8382967 - Flags: review?(felash)
Hi juliew
I have made necessary changes as per our discussion on github/irc.Have a look on PR.
Thanks
Attachment #8382967 - Attachment is obsolete: true
Attachment #8404857 - Flags: review?(felash)
Whiteboard: [mentor=:julienw] → [mentor=:julienw][lang=css]
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.
Attachment #8404857 - Flags: review?(felash) → review+
I put a NI so that I don't forget to merge when it's ready.
Flags: needinfo?(felash)
master: 18182d722d3182341f125667e2cb1045ed6d01bc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(felash)
Resolution: --- → FIXED
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!
Status: RESOLVED → REOPENED
Flags: needinfo?(felash)
Resolution: FIXED → ---
Ok, the send button needs a "overflow visible" because the counter is out of it.

I'm adding this and relanding.

Thanks Borja !
Flags: needinfo?(felash)
Attached file 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 :)
Attachment #8406831 - Flags: review?(borja.bugzilla)
Comment on attachment 8406831 [details] [review]
PR: fixed the counter

Hey Steve,

can you help to double check the fix before I reland?
Attachment #8406831 - Flags: review?(schung)
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 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.
Attachment #8406831 - Flags: review?(schung)
Attachment #8406831 - Flags: review?(borja.bugzilla)
(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 :)
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.
Flags: needinfo?(schung)
(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.
Flags: needinfo?(schung)
Comment on attachment 8406831 [details] [review]
PR: fixed the counter

Thanks again for the rebasing and fixing :)
Attachment #8406831 - Flags: review+
Github didn't want to merge so I rebased again and waiting for a green travis. Thanks !
Flags: needinfo?(felash)
master: 596ddc045ba47bd9f5f0b072343e3e2b400cc715
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(felash)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.