New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audio Player #1

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
4 participants
@angelnikolov

angelnikolov commented Dec 6, 2018

Description

This is a new component which will allow our users to drop a configurable audio player in their apps.

Dependencies

This depends on changes to fliplet-api and fliplet-file-picker

###Developer notes
To run the project in development, run the following in sequence:
npm install
npm run watch
fliplet run/fliplet publish

To run the tests run:
fliplet test

Feedback Needed

I experienced a couple of caveats when developing this component

  1. I couldn't get the cordova-plugin-media to work with offline-hosted files, which seemed like a library issue. I remote debugged that in Safari on an iOS device and none of the solutions I found in issues of that library helped. @squallstar and I discussed that and figured that for now we can leave it to online mode only (files coming from our servers)

  2. When served by fliplet-api, some audio formats like .wav, .webm, .ogg and .m4a doesn't seem to stream properly in safari at all. I tried a lot of different solutions for that issue, like setting the correct headers for streaming to safari, like here: https://stackoverflow.com/questions/34190593/ios-safari-issue-with-audio-from-server
    Or even removing ALL previous headers and setting just the ones required by Safari and again it didnt work.
    I isolated any issues with the file being encrypted, by downloading the file from the api, and directly playing it in Safari, so that can't be the issue.
    Any ideas will be warmly welcomed :)

@angelnikolov angelnikolov referenced this pull request Dec 6, 2018

Merged

Added audio #31

@tonytlwu

This comment has been minimized.

tonytlwu commented Dec 6, 2018

Love that we have tests written for this.

Though, I see a lot of ES6 syntax that IE 11 probably wouldn't support, e.g. async, await, () => {} etc. These would need to be changed.

@angelnikolov

This comment has been minimized.

angelnikolov commented Dec 6, 2018

@tonytlwu the widget.json refers to dist/interface.js which is a transpiled version of the es6 source code, so it should be fine.

@squallstar

This comment has been minimized.

squallstar commented Dec 6, 2018

@tonytlwu we've used the same setup on some other widgets like the data-source-query-provider and going forward we should use this on all widgets we produce! I can explain in more details if you or @hcarneiro require some insight on the topic.

@tonytlwu

This comment has been minimized.

tonytlwu commented Dec 6, 2018

@angelnikolov I didn't see the reference to the dist version. Excellent! 👍

@@ -0,0 +1,7 @@
<div data-fliplet-audio-player-id="{{id}}">
{{#if media.url}}

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

make sure this doesn't fail when media is undefined (e.g. cannot read property url or undefined; is handlebars affected by this?)

This comment has been minimized.

@angelnikolov

angelnikolov Dec 6, 2018

Since we don't use handlebars in strict mode, this shouldn't be an issue here. I guess they either have a mechanism for object safe navigation or just swallow the errors, either of which is fine.
https://handlebarsjs.com/reference.html
I think we can enable the strict mode, but we might enter a world of hell :)

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

@angelnikolov good finding, thanks for the reference! I like non-strict mode since it makes templates much more readable :)

done();
});

expect(interfaceBrowser)

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

missing semicolon

expect(interfaceBrowser)
});
});
describe('Build', function () {

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

put an empty line before this

(async () => {
await Fliplet();
$('.spinner-holder').removeClass('animated');
const button = $('.add-audio');

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

put an empty line before any group of variables declaration so it's more readable:

something;

const button = 1;
const foo = 'bar';
});
}

/// private methods

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

just two // are enough :)

}
}
});
const providerData = await providerInstance;

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

empty line before this

} else {
data.media = _.isEmpty(media.selectedFiles) ? media : media.selectedFiles;
}
data.media.path = null;

This comment has been minimized.

@squallstar

squallstar Dec 6, 2018

put an empty line after block (if/switch) conditions

@angelnikolov

This comment has been minimized.

angelnikolov commented Dec 6, 2018

@squallstar Addressed your comments, I will also add the eslint/jshint to the cli. Thanks!

@squallstar

This comment has been minimized.

squallstar commented Dec 6, 2018

@angelnikolov merge this and I will put it on staging!

@angelnikolov angelnikolov merged commit 2eb8a05 into master Dec 6, 2018

</div>
</div>
<p class="help-block">
When the user taps to play the audio, it will be displayed within your

This comment has been minimized.

@ibroom

ibroom Dec 7, 2018

Can we replace this text with this:

<p>This component will add an audio player to your app.</p>
<p>The audio player requires the user to be connected to the internet.</p>
<p>The audio player will continue to play when the mobile app is in the background or the user turns off their screen.</p>
<p>The audio player supports MP3 and MP4 audio files only.</p>

This comment has been minimized.

@ibroom

ibroom Dec 7, 2018

should we add something about the file types supported?

Do we limit users from selecting non-compatible file types?

ios apparently supports:
AAC (16 to 320 Kbps)
AIFF.
AAC Protected (MP4 from iTunes Store)
MP3 (16 to 320 Kbps)
MP3 VBR.
Audible (formats 2-4)
Apple Lossless.
WAV.

Android audio support: https://developer.android.com/guide/topics/media/media-formats

IE11 supports:
MP3
MP4 with AAC support
TS with AAC support

Based on this I recommend we only support MP3 or MP4

This comment has been minimized.

@angelnikolov

angelnikolov Dec 7, 2018

@ibroom I will limit the api, file-picker and the audio player to mp3 and mp4 then.

<div>
<label for="add-document">Select an audio</label>
</div>
<div>

This comment has been minimized.

@ibroom

ibroom Dec 7, 2018

enhancement idea - we can offer the ability for the user to paste in a link to an existing audio file instead of requiring the user to upload?

enhancement idea - like the online video component, can we use embedly to find the embed code for audio providers like soundcloud or spotify? See audio providers at https://embed.ly/providers and review the online video component for the embedly code

This comment has been minimized.

@angelnikolov

angelnikolov Dec 7, 2018

I think we discussed the url option with @tonytlwu and @squallstar and it is a nice idea. I can add it in and will afterwards research the embedly functionality. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment