986510 - [Messages app] Replace menu icon with settings icon when no messages in app

Status: RESOLVED FIXED

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

Description

[ayman maat :maat]

When the messages app contains messages the icon in the top right hand corner of the apps inbox triggers a menu that contains the following options:

      - Delete Messages
      - Settings

When the messages app contains no messages the icon in the top right hand corner of the apps inbox triggers a menu that contains the following options:

      - Settings

…in this instance having a menu with a single item in it is inefficient. in view of this it would be appropriate to replace the ‘list icon’ in the top right hand corner of the inbox with a ‘settings icon’ that directly opens message settings for the instance when there are no messages in the app.

Comment 1

[Noemí Freire (:noemi)]

adding to backlog to be properly prioritized. Thanks!

Comment 2

[Jeevan M R (:jee1mr)]

I'm interested to take this up. 
But is it approved by the UX Team?

Comment 3

[Steve Chung [:steveck]]

Ni UX about this behavior

Comment 4

[Jenny Lee]

Hi, I think the change makes sense. Please do as ayman suggested, thanks!

Comment 5

[Fang Shih [:fang]]

Attachment: icon_setting.zip

Attached the setting icons,Thanks!

Comment 6

[Jeevan M R (:jee1mr)]

(In reply to Fang Shih from comment #5)
> Created attachment 8478116 [details]
> icon_setting.zip
> 
> Attached the setting icons,Thanks!

Thanks :)

Comment 7

[Julien Wajsberg [:julienw]]

Hey Jeevan, please ask any question here and we'll be glad to help you. We're also on IRC (irc.mozilla.org/#gaia), so feel free to come over.
Thanks !

Comment 8

[kumar rishav (:rishav_)]

jeevan seems unavailable. resetting to nobody.

Hey vishnu
This might interest you :)

Comment 9

[Vishnu Teja [:ythej]]

Hi rishav,
Yup, I am interested in this bug.Feel free in assigning it to me if jeevan is unavailable.

Thank for searching it for me. :)

Comment 10

[Vishnu Teja [:ythej]]

Attachment: pull request for the bug 986510

Hi Julien,
I couldn't figure out with integration tests.Completely unrelated tests are getting failed in treeherder and they are not same when they are runned locally.I think I don't have the right to restart a particular test's in treeherder.Can you please check it and restart the failed test?
Thank you.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

It works and does the job, but I think we should add a class to the article instead. NI Oleg to answer the question I added on the PR.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Replied on github, thanks!

Comment 13

[Julien Wajsberg [:julienw]]

So, I learnt things about gaia-header and scoped styles today. It looks like inner scoped styles have precedence over less inner scoped styles and non-scoped styles. This is all in http://dev.w3.org/csswg/css-cascade/#cascading and see bug 904999 too.

So we have 3 solutions:
* we can use !important for "display: none"
* we can use the "hide" class like you did initially (and this is quite the same because this uses !important too)
* we can change the existing button data-action, but in that case we need to have a click handler that can distinguish between the 2 cases.

Oleg, what do you think? I'd say the 1st solution is the cleanest (because we can have a nice class that triggers everything) while the second solution is the easiest.

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> So, I learnt things about gaia-header and scoped styles today. It looks like
> inner scoped styles have precedence over less inner scoped styles and
> non-scoped styles. This is all in
> http://dev.w3.org/csswg/css-cascade/#cascading and see bug 904999 too.
> 
> So we have 3 solutions:
> * we can use !important for "display: none"
> * we can use the "hide" class like you did initially (and this is quite the
> same because this uses !important too)
> * we can change the existing button data-action, but in that case we need to
> have a click handler that can distinguish between the 2 cases.
> 
> Oleg, what do you think? I'd say the 1st solution is the cleanest (because
> we can have a nice class that triggers everything) while the second solution
> is the easiest.

Yep, the first option is the best in my opinion. IIRC gaia-header scoped styling may be changed once we get ":host\::content" selector support and if so, we'll just need to remove "!important".

Comment 15

[Vishnu Teja [:ythej]]

But I think we need to use the '!important' for displaying the same settings-button when the class is add to the panel and also when we need to hide the options-button.Are we going to use three '!important' ?
Thank you.

Comment 16

[Julien Wajsberg [:julienw]]

I think it's the best solution even if it's not perfect :/

Comment 17

[Vishnu Teja [:ythej]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Hi Julien,
Implemented the fix with the 1st way you suggested.I couldn't decide with integration tests as it is very difficult to know which among them are exactly failing.Please let me know if we need integration test for this case.
Thank you.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Some small nits and I think this should be good soon !
Thanks and sorry again for the delay

Comment 19

[Vishnu Teja [:ythej]]

Julien, thanks a lot for giving the review inspite of having a very tough schedule :)

Comment 20

[Vishnu Teja [:ythej]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, I rebased it :).
Thank you !

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Just one nit is left :)

Comment 22

[Vishnu Teja [:ythej]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, Thank you :)

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

2 comments... but you should really check the test results before asking for review !

Please add your changes to a separate commit to ease the next review.

Comment 24

[Vishnu Teja [:ythej]]

Hi Julien,

The tests running fine locally but the tests are not getting started in treeherder.

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=44a9d97254c6 .

Thank you :)

Comment 25

[Vishnu Teja [:ythej]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, all tests passed this time !

Thank you :)

Comment 26

[Julien Wajsberg [:julienw]]

Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

r=me

please remove the file npm-debug.log from the patch, squash your commits together, and add the "checkin-needed" keyword when you're ready !

Thanks a lot !

Comment 27

[Vishnu Teja [:ythej]]

Julien, sorry for the delay.

Thank you :)

Comment 28

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/06ddd771543753b65315353131c2114149a31cdf