Closed Bug 986510 Opened 6 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: maat, Assigned: ythej)

References

Details

Attachments

(2 files)

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.
adding to backlog to be properly prioritized. Thanks!
blocking-b2g: --- → backlog
I'm interested to take this up. 
But is it approved by the UX Team?
Ni UX about this behavior
Flags: needinfo?(jelee)
Hi, I think the change makes sense. Please do as ayman suggested, thanks!
Flags: needinfo?(jelee)
Attached file icon_setting.zip
Attached the setting icons,Thanks!
(In reply to Fang Shih from comment #5)
> Created attachment 8478116 [details]
> icon_setting.zip
> 
> Attached the setting icons,Thanks!

Thanks :)
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 !
Assignee: nobody → 14.jeevan
jeevan seems unavailable. resetting to nobody.

Hey vishnu
This might interest you :)
Assignee: 14.jeevan → nobody
Flags: needinfo?(yvtheja)
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. :)
Flags: needinfo?(yvtheja)
Assignee: nobody → yvtheja
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.
Attachment #8555714 - Flags: feedback?(felash)
Attachment #8555714 - Flags: feedback?(rishav006)
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.
Flags: needinfo?(azasypkin)
Attachment #8555714 - Flags: feedback?(rishav006)
Attachment #8555714 - Flags: feedback?(felash)
Replied on github, thanks!
Flags: needinfo?(azasypkin)
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.
Flags: needinfo?(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".
Flags: needinfo?(azasypkin)
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.
I think it's the best solution even if it's not perfect :/
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.
Attachment #8555714 - Flags: review?(felash)
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
Attachment #8555714 - Flags: review?(felash)
Julien, thanks a lot for giving the review inspite of having a very tough schedule :)
Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, I rebased it :).
Thank you !
Attachment #8555714 - Flags: review?(felash)
Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Just one nit is left :)
Attachment #8555714 - Flags: review?(felash)
Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, Thank you :)
Attachment #8555714 - Flags: review?(felash)
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.
Attachment #8555714 - Flags: review?(felash)
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 :)
Flags: needinfo?(felash)
Comment on attachment 8555714 [details] [review]
pull request for the bug 986510

Julien, all tests passed this time !

Thank you :)
Flags: needinfo?(felash)
Attachment #8555714 - Flags: review?(felash)
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 !
Attachment #8555714 - Flags: review?(felash) → review+
Julien, sorry for the delay.

Thank you :)
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1138517
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.