1212338 - Move the user's "availability" option to the settings menu as a "turn notifications on/off"

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mark Banner (:standard8)]

As part of the user journey rework, we need to move and adjust the availability option. This depends on bug 1212074.

See the user story for more detail.

Comment 1

[Manuel Casas Barrado [:mancas]]

Attachment: Move the user's "availability" option to the settings menu as a "turn notifications on/off"

Hey Mark, could you take a look at the patch when you get a chance? Let me know if there is something wrong

Comment 2

[Manuel Casas Barrado [:mancas]]

Attachment: notifications_onoff.png

Comment 3

[Pau Masiá [:Pau]]

Comment on attachment 8671368 [details]
notifications_onoff.png

You're almost done, Manu! There's only 1 thing left:
Make sure the divider fits the whole width of the menu panel, it seems like there's 1 px left on the right side.

Thanks!

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Move the user's "availability" option to the settings menu as a "turn notifications on/off"

It seems that every entry in the dropdown menu has a transparent border so this patch takes that problem into account. Now the divider fits the whole width

Comment 5

[Manuel Casas Barrado [:mancas]]

Attachment: notifications_onoff.png

Comment 6

[Pau Masiá [:Pau]]

Comment on attachment 8671425 [details]
notifications_onoff.png

Cool, Thanks!

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8671423 [details] [diff] [review]
Move the user's "availability" option to the settings menu as a "turn notifications on/off"

Review of attachment 8671423 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nice! There's still a little more tidy up that could be done though.

::: browser/components/loop/content/css/panel.css
@@ -676,5 @@
> -  margin-right: -3px;
> -}
> -
> -.status-available:before {
> -  background-image: url("../shared/img/icons-16x16.svg#status-available");

Please can you also remove these icons (#status-available and #status-unavailable) from the svg file and remove the references from the ui-showcase please?

@@ +693,5 @@
>    bottom: 1.1em;
>    right: 14px;
>  }
>  
> +.settings-menu .entries-divider {

Can you make this either .settings-menu > .dropdown-menu > .entries-divider or just .entries-divider?

Child selectors have better performance the descendant ones, and I realise the existing css isn't ideal, but we're at least moving the new css to the better styles.

You could make it just .entries-divider, although we don't currently have a use for it on more than one menu, I could see it being potentially useful in the future.

::: browser/components/loop/content/js/panel.jsx
@@ +195,5 @@
>      },
>  
>      mixins: [sharedMixins.DropdownMenuMixin(), sharedMixins.WindowCloseMixin],
>  
> +

nit: no need for the extra blank line here.

@@ +249,5 @@
>               ref="menu-button"
>               title={mozL10n.get("settings_menu_button_tooltip")} />
>            <ul className={cx({"dropdown-menu": true, hide: !this.state.showMenu})}>
>              <SettingsDropdownEntry
> +                extraCSSClass="entry-settings-notificiations entries-divider"

nit: s/notificiations/notifications/

(fix in tests as well).

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +398,5 @@
>          sinon.assert.calledOnce(fakeWindow.close);
>        });
>      });
>  
> +    describe("Toggle Notifications", function() {

Can you make this a subsection of the SettingsDropdown please? I think it fits better there.

@@ +412,5 @@
> +      beforeEach(function() {
> +        view = mountTestComponent();
> +      });
> +
> +      it("should toggle mozLoop.doNotDisturb to false", function() {

Can you also add a test to check that it hides the dropdown menu please?

Comment 8

[Manuel Casas Barrado [:mancas]]

Attachment: Move the user's "availability" option to the settings menu as a "turn notifications on/off"

Mark, I've fixed all the minor issues that you found. Please review the patch again in order to start uploading the next patches that depend on this one.

Thanks!

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8672944 [details] [diff] [review]
Move the user's "availability" option to the settings menu as a "turn notifications on/off"

Review of attachment 8672944 [details] [diff] [review]:
-----------------------------------------------------------------

That's great. r=Standard8

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/990409ca46a3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/990409ca46a3