Closed Bug 1212338 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: standard8, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

User Story

Acceptance criteria:

- Available/do not disturb menu removed from panel footer.
- The guest/email address moves to the left-hand side of the footer.
- There is a "Turn notifications off" that toggles with "Turn notifications on" option in the settings menu
- Activating the option toggles the existing doNotDisturb flag on navigator.mozLoop
- Old css & strings are removed
- Any relevant views removed from the ui-showcase.

Attachments

(2 files, 3 obsolete files)

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.
Rank: 12
Whiteboard: [web sharing]
Blocks: 1212340
Assignee: nobody → b.mcb
Hey Mark, could you take a look at the patch when you get a chance? Let me know if there is something wrong
Attachment #8671356 - Flags: review?(standard8)
Attached image notifications_onoff.png (obsolete) —
Attachment #8671368 - Flags: ui-review?(b.pmm)
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!
Attachment #8671368 - Flags: ui-review?(b.pmm) → ui-review-
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
Attachment #8671356 - Attachment is obsolete: true
Attachment #8671356 - Flags: review?(standard8)
Attachment #8671423 - Flags: review?(standard8)
Attached image notifications_onoff.png
Attachment #8671368 - Attachment is obsolete: true
Attachment #8671425 - Flags: ui-review?(b.pmm)
Comment on attachment 8671425 [details]
notifications_onoff.png

Cool, Thanks!
Attachment #8671425 - Flags: ui-review?(b.pmm) → ui-review+
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?
Attachment #8671423 - Flags: review?(standard8) → review-
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!
Attachment #8671423 - Attachment is obsolete: true
Attachment #8672944 - Flags: review?(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
Attachment #8672944 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/990409ca46a3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.