Closed Bug 1104930 Opened 5 years ago Closed 5 years ago

Create a mixin for handling updating of the video container

Categories

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

defect
Points:
2

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan
Blocking Flags:
backlog tech-debt

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [tech-debt])

Attachments

(1 file)

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?
backlog: Fx37? → Fx38?
backlog: Fx38? → tech-debt
Priority: -- → P4
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/integration/fx-team/rev/68fbd452c2d9
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.