[Notifications] »Clear all« and header shows even with no notifications

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jan-Christoph Borchardt, Assigned: Tom Leese)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=timdream])

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
timdream
: review+
timdream
: ui-review+
timdream
: feedback+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux armv7l) AppleWebKit/536.11 (KHTML, like Gecko) Ubuntu/12.04 Chromium/20.0.1132.47 Chrome/20.0.1132.47 Safari/536.11

Steps to reproduce:

Have no notifications and bring down the notification tray.


Actual results:

The »Notifications« header is there, and »Clear all« is displayed on the right. Albeit greyed out, it’s a bit confusing and seems like there should be notifications there.


Expected results:

Instead of displaying the header like that with the »clear all« on the right, it should probably be »No notifications« or something similar. As feedback that there’s nothing there.
(Assignee)

Comment 1

4 years ago
Created attachment 817298 [details] [review]
pull request

I followed the instructions on https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Gaia/Hacking#Submitting_a_patch to fix this bug. I wasn't sure about how to file or assign myself to the bug though, so I just fixed it and have submitted the pull request.

Sorry if I have got this wrong somehow!
Attachment #817298 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #817298 - Flags: review? → review?(timdream)
Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 817298 [details] [review]
pull request

This is an excellent patch and thank you for patching Gaia.
Besides the comment on Github, you would also need to patch unit tests and make sure Travis-CI runs green:

https://travis-ci.org/mozilla-b2g/gaia/jobs/12581972#L1522

I will flag UX in the next comment to see if this is the desired interaction.
Attachment #817298 - Flags: review?(timdream) → feedback+
Hi UX team, leese.thomas81@gmail.com here made a patch so what when there no notification, The "Notifications - clear all" header will be replaced with "No notifications". Is that the desired interaction? Or should we simply hide the "clear all" button, or the header itself?

I will help leese.thomas81@gmail.com to come up another patch when the interaction is decided.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(firefoxos-ux-bugzilla)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> Hi UX team, leese.thomas81@gmail.com here made a patch so what when there no
> notification, The "Notifications - clear all" header will be replaced with
> "No notifications". Is that the desired interaction? Or should we simply
> hide the "clear all" button, or the header itself?

Sorry, that's not exactly what his patch did. His patch would make the header show "Notifications - No notifications" if I didn't misread it.
(Assignee)

Comment 5

4 years ago
Comment on attachment 817298 [details] [review]
pull request

I think I've fixed all the issues noted (except for the UX one) in the original patch.
Attachment #817298 - Flags: review?(timdream)
Comment on attachment 817298 [details] [review]
pull request

Wonderful, thanks.

I am setting ui-review? so that people won't land it prematurely.
Attachment #817298 - Flags: ui-review?
Attachment #817298 - Flags: review?(timdream)
Attachment #817298 - Flags: review+
Assignee: nobody → leese.thomas81

Updated

4 years ago
Flags: needinfo?(rmacdonald)
Flags: needinfo?(jsavory)

Comment 7

4 years ago
Flagging Rob for a Sys FE check of this patch, especially of how it shows "No notifications."
Flags: needinfo?(jsavory)
Flags: needinfo?(firefoxos-ux-bugzilla)
Yes, the header should change from "Notification" to "No notifications" when the user has no items in the utility tray.

However, the "Clear All" button should remain where in place and instead should grey out to display that it is no longer actionable to users.
Flags: needinfo?(rmacdonald)
(Assignee)

Comment 9

4 years ago
Comment on attachment 817298 [details] [review]
pull request

I've changed how my patch works to follow the UX guidelines as outlined above.
Comment on attachment 817298 [details] [review]
pull request

https://travis-ci.org/mozilla-b2g/gaia/jobs/13204733#L1540

There is a test failure... could you fix it?
Attachment #817298 - Flags: ui-review?
Attachment #817298 - Flags: ui-review+
Attachment #817298 - Flags: review+
Flags: needinfo?(leese.thomas81)
Do remember to reset the review? flag to me please.
(Assignee)

Comment 12

4 years ago
Comment on attachment 817298 [details] [review]
pull request

Fixed the issue with the tests, passes in Travis CI now.
Attachment #817298 - Flags: review?(timdream)
(Assignee)

Updated

4 years ago
Flags: needinfo?(leese.thomas81)
Attachment #817298 - Flags: review?(timdream) → review+
master: https://github.com/mozilla-b2g/gaia/commit/04c1e38c5186cb06f6e61c4b80ab1aff3d4e3ae9

Thank you again for your contribution!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=timdream]
You need to log in before you can comment on or make changes to this bug.