Skip to content
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

x-audio #269

Closed
wants to merge 230 commits into from
Closed

x-audio #269

wants to merge 230 commits into from

Conversation

fenglish
Copy link
Contributor

@fenglish fenglish commented Apr 17, 2019

🚧 WIP 🚧
Audio player

audio-player-2

Asuka.Ochi and others added 12 commits April 18, 2019 15:30
* configure expanded + minimised stories

* setup styling

* move to css grid layout

* More descriptive var names

* Fix eslint errors

* Update snapshot

* Update snapshot again

* Tidy up a little bit
- nasty, need to find a cleaner way to do this
- also notify state changes
Only care about state/props changes that are relevant
Otherwise this can fire false re-renders which creates unnecessary noise.
@apaleslimghost apaleslimghost added the component A new component in development label Apr 29, 2019
@@ -0,0 +1,11 @@
{
"name": "x-audio",
"main": "dist/Audio.es5.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be the ES6 bundle as we always transform the contents of Bower components (Origami components are ES6)

"@financial-times/x-engine": "file:../../packages/x-engine",
"classnames": "^2.2.6",
"hammerjs": "^2.0.8",
"prop-types": "^15.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a peer dependency?

Copy link
Contributor

@i-like-robots i-like-robots Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may also be desirable to add some extra configuration to our build step to ensure they are removed in production.... but that's for another time.


This module can be used to play an audio file via pluggable engines/players.

It comes bundled with a web audio player (via [HTMLAudioElement](https://developer.mozilla.org/en-US/docs/Web/API/HTMLAudioElement)), but can be used with another engine such as in the native layer. The bundled engine uses a [Redux](https://redux.js.org/) to manage state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a Redux to manage state

-> "uses Redux to manage state"

@i-like-robots
Copy link
Contributor

This is too big for me to do an in-depth review of, but it looks good! It appears to be documented and tested well and I trust that you had a good feedback loop in the team.

However, this is not on FT.com and I'm not sure if there are any plans to make it so which means it is not technically an x- component.

As discussed IRL I don't think that necessarily has to be a problem (cc #263) but some reassurance that the component has been tested and would work on FT.com in its current form would be a bonus - has it been tried?

@debugwand
Copy link
Contributor

we've not as there would be work to get it all set up on dot com, so whoever comes back to it would need to do that. for example, the slider for the timeline is set up for touch devices and would need mouse support added in.

@edds
Copy link
Contributor

edds commented Oct 22, 2019

This PR is being closed due to inactivity as part of a Pull Request Bankruptcy. If you feel it’s still useful or relevant please feel free to re-open it.

@edds edds closed this Oct 22, 2019
@robsquires robsquires reopened this Oct 29, 2019
Base automatically changed from master to main February 8, 2021 13:05
@emortong emortong requested a review from a team as a code owner February 8, 2021 13:05
@rowanbeentje
Copy link
Contributor

This component was never added to ft.com, and because we needed tighter integration within the app*, we have imported and adopted it as a player component within ft-app. So I'm going to close this PR :)

(* Note this would have been perfectly possibly with x-interaction etc, but we could simplify the implementation if we knew it was app-only!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component A new component in development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants