Create a mixin for handling updating of the video container

RESOLVED FIXED in mozilla38

Status

Hello (Loop)
Client
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla38
Points:
2

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment)

In all our media based views we've got updateVideoContainer to update the video on resize or orientation change.

We should create a mixin to share that.
backlog: --- → Fx37?

Updated

4 years ago
backlog: Fx37? → Fx38?

Updated

4 years ago
backlog: Fx38? → tech-debt
Priority: -- → P4
Created attachment 8550247 [details] [diff] [review]
Create a mixin for handling updating of the video container.

I've pulled this forward, as it should make handling the changes in bug 1093780 easier, as it'll centralize the locations of these setup values.
Attachment #8550247 - Flags: review?(nperriault)
Assignee: nobody → standard8
Blocks: 1093780
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Priority: P4 → P2
Comment on attachment 8550247 [details] [diff] [review]
Create a mixin for handling updating of the video container.

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

Looks great, much cleaner. r=NiKo` with minor comments addressed.

::: browser/components/loop/content/shared/js/mixins.js
@@ +186,5 @@
> +     * Returns the default configuration for publishing media on the sdk.
> +     *
> +     * @param {Boolean} publishVideo True to publish video initially
> +     */
> +    getDefaultPublisherConfig: function(publishVideo) {

Let's use an options object to avoid falling in the Boolean trap[0].

[0]: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +93,5 @@
>      fakeWindow = {
>        navigator: { mozLoop: fakeMozLoop },
> +      close: sinon.stub(),
> +      addEventListener: sinon.stub(),
> +      removeEventListener: sinon.stub()

As we're not really using these two stubs, maybe we could just use a noop function.

::: browser/components/loop/test/shared/mixins_test.js
@@ +207,5 @@
> +      rootObject = {
> +        events: {},
> +        addEventListener: function(eventName, listener) {
> +          this.events[eventName] = listener;
> +        },

Love this!

@@ +215,5 @@
> +      };
> +
> +      sharedMixins.setRootObject(rootObject);
> +
> +

Nit: One blank line too many.
Attachment #8550247 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/mozilla-central/rev/68fbd452c2d9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.