Bugfix/use media capabilities #51

anton-karlovskiy · 2020-03-16T18:39:14Z

I've created this PR to highlight potential updates on naming and docs in media capabilities hook.

…ntext

stramel · 2020-03-16T19:02:07Z

media-capabilities/index.js

    supported,
-    hasMediaConfig: !!mediaConfig
+    hasMediaDecodingConfig: !!mediaDecodingConfig


What's the point of this? This seems like it should just be an internal variable.

@wingleung cc @addyosmani

I agree with @stramel.
I guess try/catch block should wrap navigator.mediaCapabilities.decodingInfo(mediaDecodingConfig) for handling exceptions without hasMediaDecodingConfig.

A TypeError is raised if the MediaConfiguration passed to the decodingInfo() method is invalid, either because the type is not video or audio, the contentType is not a valid codec MIMME type, the media decoding configuration is not a valid value for the media decoding type, or any other error in the media configuration passed to the method, including omitting values required in the media decoding configuration.

From MediaCapabilities.decodingInfo()

stramel · 2020-03-16T19:02:43Z

media-capabilities/index.js

-  mediaCapabilities = (mediaCapabilities.supported && mediaCapabilities.hasMediaConfig)
-    ? navigator.mediaCapabilities.decodingInfo(mediaConfig)
+  mediaCapabilitiesInfo = (mediaCapabilitiesInfo.supported && mediaCapabilitiesInfo.hasMediaDecodingConfig)
+    ? navigator.mediaCapabilities.decodingInfo(mediaDecodingConfig)


This still doesn't take into account the fact this returns a promise.

WDYT about using an API like,

const { supported, decodingInfo, encodingInfo } = useMediaCapabilities(initialState)

either that or we need to wrap the async calls and expose them to show the state of the calls.

@wingleung cc @addyosmani

What are your thoughts?
I agree with @stramel's API suggestion. I think it looks more descriptive rather than just mediaCapabilitiesInfo.
For now, I think it could be like so.

const { supported, smooth, powerEfficient } = useMediaCapabilities(initialState);

stramel · 2020-03-16T19:03:23Z

media-capabilities/index.js

    };

-  return {mediaCapabilities};
+  return {mediaCapabilitiesInfo};


Shouldn't this be {...mediaCapabilitiesInfo}?

@wingleung cc @addyosmani

I agree with @stramel.

anton-karlovskiy · 2020-03-16T21:14:03Z

README.md

@@ -160,16 +160,16 @@ const { deviceMemory } = useMemoryStatus(initialMemoryStatus);

 ### Media Capabilities

-`useMediaCapabilities` utility for adapting based on the user's device media capabilities.
+`useMediaCapabilitiesDecodingInfo` utility for adapting based on the user's device media capabilities.


@wingleung cc @addyosmani

Looking into the implementation of this hook, I found that it just deals with decoding media capabilities, not all media capabilities (encoding and decoding). So I'd like to suggest updating its name to useMediaCapabilitiesDecodingInfo.
I've got inspired by this MediaCapabilities.decodingInfo().

anton-karlovskiy · 2020-03-16T21:18:31Z

README.md


-const webmMediaConfig = {
+const webmMediaDecodingConfig = {


@wingleung cc @addyosmani

According to MediaCapabilities.decodingInfo(), the argument type of mediaCapabilities.decodingInfo(MediaDecodingConfiguration) is MediaDecodingConfiguration.
So I'd like to suggest updating to webmMediaDecodingConfig.

anton-karlovskiy · 2020-03-16T21:25:22Z

README.md


 const MyComponent = ({ videoSources }) => {
-  const { mediaCapabilities } = useMediaCapabilities(webmMediaConfig, initialMediaCapabilities);
+  const { mediaCapabilitiesInfo } = useMediaCapabilitiesDecodingInfo(webmMediaDecodingConfig, initialMediaCapabilitiesInfo);


@wingleung cc @addyosmani

According to MediaCapabilities.decodingInfo(), the return value of mediaCapabilities.decodingInfo(MediaDecodingConfiguration) is a promise fulfilling with a MediaCapabilitiesInfo interface containing three Boolean attributes: supported, smooth, and powerEfficient.
So I'd like to suggest updating to mediaCapabilitiesInfo and initialMediaCapabilitiesInfo.

the hook itself will return an object that will contain the 3 booleans, as mentioned in https://github.com/GoogleChromeLabs/react-adaptive-hooks/pull/51/files?file-filters%5B%5D=.js&file-filters%5B%5D=.md#r393336802

anton-karlovskiy · 2020-03-16T21:32:23Z

README.md

@@ -200,7 +200,9 @@ const MyComponent = ({ videoSources }) => {
 };
 ```

-This hook accepts a [media configuration](https://developer.mozilla.org/en-US/docs/Web/API/MediaConfiguration) object argument and an optional `initialMediaCapabilities` object argument, which can be used to provide a `mediaCapabilities` state value when the user's browser does not support the relevant [Media Capabilities API](https://developer.mozilla.org/en-US/docs/Web/API/Media_Capabilities_API) or no media configuration was given.
+`mediaCapabilitiesInfo` value contains the three Boolean properties `supported`, `smooth`, and `powerefficient`, which describe whether decoding the media described would be supported, smooth, and powerefficient.


@wingleung cc @addyosmani

I think we should explain what we can get from mediaCapabilitiesInfo as we have done in other hooks.
e.g. effectiveConnectionType values can be slow-2g, 2g, 3g, or 4g.

anton-karlovskiy · 2020-03-16T21:52:14Z

README.md

-This hook accepts a [media configuration](https://developer.mozilla.org/en-US/docs/Web/API/MediaConfiguration) object argument and an optional `initialMediaCapabilities` object argument, which can be used to provide a `mediaCapabilities` state value when the user's browser does not support the relevant [Media Capabilities API](https://developer.mozilla.org/en-US/docs/Web/API/Media_Capabilities_API) or no media configuration was given.
+`mediaCapabilitiesInfo` value contains the three Boolean properties `supported`, `smooth`, and `powerefficient`, which describe whether decoding the media described would be supported, smooth, and powerefficient.
+
+This utility accepts a [MediaDecodingConfiguration](https://developer.mozilla.org/en-US/docs/Web/API/MediaDecodingConfiguration) object argument and an optional `initialMediaCapabilitiesInfo` object argument, which can be used to provide a `mediaCapabilitiesInfo` state value when the user's browser does not support the relevant [Media Capabilities API](https://developer.mozilla.org/en-US/docs/Web/API/Media_Capabilities_API) or no media configuration was given.


@wingleung cc @addyosmani

According to MediaCapabilities.decodingInfo():

A TypeError is raised if the MediaConfiguration passed to the decodingInfo() method is invalid, either because the type is not video or audio, the contentType is not a valid codec MIMME type, the media decoding configuration is not a valid value for the media decoding type, or any other error in the media configuration passed to the method, including omitting values required in the media decoding configuration.

From MediaCapabilities.decodingInfo()

anton-karlovskiy · 2020-03-16T22:04:01Z

media-capabilities/index.js

    supported,
-    hasMediaConfig: !!mediaConfig
+    hasMediaDecodingConfig: !!mediaDecodingConfig


@wingleung cc @addyosmani

I agree with @stramel.
I guess try/catch block should wrap navigator.mediaCapabilities.decodingInfo(mediaDecodingConfig) for handling exceptions without hasMediaDecodingConfig.

A TypeError is raised if the MediaConfiguration passed to the decodingInfo() method is invalid, either because the type is not video or audio, the contentType is not a valid codec MIMME type, the media decoding configuration is not a valid value for the media decoding type, or any other error in the media configuration passed to the method, including omitting values required in the media decoding configuration.

From MediaCapabilities.decodingInfo()

anton-karlovskiy · 2020-03-16T22:08:53Z

media-capabilities/index.js

-  mediaCapabilities = (mediaCapabilities.supported && mediaCapabilities.hasMediaConfig)
-    ? navigator.mediaCapabilities.decodingInfo(mediaConfig)
+  mediaCapabilitiesInfo = (mediaCapabilitiesInfo.supported && mediaCapabilitiesInfo.hasMediaDecodingConfig)
+    ? navigator.mediaCapabilities.decodingInfo(mediaDecodingConfig)


@wingleung cc @addyosmani

What are your thoughts?
I agree with @stramel's API suggestion. I think it looks more descriptive rather than just mediaCapabilitiesInfo.
For now, I think it could be like so.

const { supported, smooth, powerEfficient } = useMediaCapabilities(initialState);

anton-karlovskiy · 2020-03-16T22:13:03Z

media-capabilities/index.js

    };

-  return {mediaCapabilities};
+  return {mediaCapabilitiesInfo};


@wingleung cc @addyosmani

I agree with @stramel.

anton-karlovskiy · 2020-03-16T22:23:50Z

media-capabilities/media-capabilities.test.js

@@ -16,7 +16,7 @@



@wingleung cc @addyosmani

Just in the scope of naming I'd like to suggest updating like the following.

anton-karlovskiy · 2020-03-20T23:06:32Z

I'm closing this PR in favor of #52.

Thank you. @stramel @wingleung

anton-karlovskiy added 7 commits March 11, 2020 12:02

doc: improved code styling in readme

371d010

infra: uninstalled react-dom

9940103

chore: cleaned up

f64860d

infra: extracted supported from hook in useMediaCapabilities

06c2a0a

doc: cleaned code snippet for useMediaCapabilities in README

def7edd

Merge remote-tracking branch 'upstream/master'

5f7a6b4

[infra] updated naming for media capabilities hook considering its co…

c232111

…ntext

stramel suggested changes Mar 16, 2020

View reviewed changes

anton-karlovskiy mentioned this pull request Mar 16, 2020

async mediaCapabilities #52

Merged

[doc] described mediaCapabilitiesInfo value

aa6fa12

anton-karlovskiy commented Mar 16, 2020

View reviewed changes

anton-karlovskiy force-pushed the bugfix/use-media-capabilities branch from e2bda82 to aa6fa12 Compare March 16, 2020 22:26

anton-karlovskiy added 2 commits March 18, 2020 18:44

fix: typo

1485417

doc: useMediaCapabilitiesDecodingInfo added in UMD section

fdb2c3d

anton-karlovskiy closed this Mar 20, 2020

anton-karlovskiy deleted the bugfix/use-media-capabilities branch March 20, 2020 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/use media capabilities #51

Bugfix/use media capabilities #51

anton-karlovskiy commented Mar 16, 2020

stramel Mar 16, 2020

anton-karlovskiy Mar 16, 2020 •

edited

Loading

stramel Mar 16, 2020

stramel Mar 16, 2020

anton-karlovskiy Mar 16, 2020

stramel Mar 16, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020

wingleung Mar 17, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020 •

edited

Loading

anton-karlovskiy Mar 16, 2020 •

edited

Loading

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy Mar 16, 2020

anton-karlovskiy commented Mar 20, 2020 •

edited

Loading

Bugfix/use media capabilities #51

Bugfix/use media capabilities #51

Conversation

anton-karlovskiy commented Mar 16, 2020

Choose a reason for hiding this comment

anton-karlovskiy Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-karlovskiy Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

anton-karlovskiy Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-karlovskiy commented Mar 20, 2020 • edited Loading

anton-karlovskiy Mar 16, 2020 •

edited

Loading

anton-karlovskiy Mar 16, 2020 •

edited

Loading

anton-karlovskiy Mar 16, 2020 •

edited

Loading

anton-karlovskiy commented Mar 20, 2020 •

edited

Loading