Ndlano/issue#474 lydvelger som egen pakke #76
Conversation
…m:NDLANO/frontend-packages into NDLANO/Issue#474-Lydvelger-som-egen-pakke
| return ( | ||
| <div {...classes()}> | ||
| <audio autoPlay controls onPlay={!audioSource && this.loadAudio}> | ||
| {' '}{audioSource} |
There was a problem hiding this comment.
Hvorfor {' '}{audioSource} ?
There was a problem hiding this comment.
Med state endringen ovenfor kan du ta ha
<source src={this.state.src} type={this.state.type} />
| } | ||
|
|
||
| AudioComponent.propTypes = { | ||
| audio: PropTypes.object.isRequired, |
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| queryObject: this.props.queryObject, |
There was a problem hiding this comment.
bare props, ikke this.props
|
|
||
| return ( | ||
| <div {...classes('form')}> | ||
| <select onChange={this.handleLanguageChange}> |
There was a problem hiding this comment.
Tror ikke vi burde ha det her. Dette er noe som bestemmes av locale som blir sendt inn av den frontenden som bruker denne pakken.
| } | ||
|
|
||
| AudioSearchForm.propTypes = { | ||
| queryObject: PropTypes.object, |
There was a problem hiding this comment.
Proptypes.shape her også.
| }; | ||
|
|
||
| AudioSearchForm.defaultProps = { | ||
| query: '', |
There was a problem hiding this comment.
Er query en prop? Ser den ikke i valideringen i hvertfall
| import BEMHelper from 'react-bem-helper'; | ||
| import { getLicenseByAbbreviation } from 'ndla-licenses'; | ||
| import { Button, LicenseIconList } from 'ndla-ui'; | ||
|
|
| prefix: 'c-', | ||
| }); | ||
|
|
||
| class AudioSearchResult extends Component { |
There was a problem hiding this comment.
Her håndterer du ingen komponent state eller bruker noen livsyklus metoder så denne kan være en stateless component, feks slik
const AudioSearchResult = (props) => {
return (
<div key={audio.id} {...classes('list-item', 'search-result')}>
//resten her
</div>
)
}
|
|
||
| @import "global/settings/settings.config"; | ||
| @import "global/settings/settings.colors"; | ||
| $xxlarge-viewport-max: "max-width: 1400px)"; |
There was a problem hiding this comment.
Disse viewportsa trenger du ikke hvis du ikke skal bruke dem.
| .fetchAudio(this.props.audio.id) | ||
| .then(result => { | ||
| this.setState({ | ||
| audioSource: ( |
There was a problem hiding this comment.
Her ville jeg heller ha satt
this.setState({src: result.audioFile.url, type: result.audioFile.mimeType});og brukt dette i source taggen lenger ned.
|
Har lagt inn en del kommentarer nå. Det er også fint om du kan lage README.md. |
| }, | ||
| "dependencies": { | ||
| "defined": "1.0.0", | ||
| "react-bem-helper": "^1.2.0" |
There was a problem hiding this comment.
Trengs vel bare som devDep.
|
|
||
| // The 'audio' tag is implemented with a download button in Chrome > 58. | ||
| // Make it so that this button doesn't show. | ||
| audio::-webkit-media-controls { |
There was a problem hiding this comment.
Dette vil jo bare funke på webkit støtta browsere da?
| import BEMHelper from 'react-bem-helper'; | ||
|
|
||
| const classes = new BEMHelper({ | ||
| name: 'audio-component', |
There was a problem hiding this comment.
prefix c står for component, så ville heller hatt audio-preview eller noe sånt i stedet.
| "ndla-licenses": "^0.0.7", | ||
| "ndla-ui": "^0.7.4", | ||
| "ndla-util": "^0.1.3", | ||
| "defined": "1.0.0", |
There was a problem hiding this comment.
defined må være dependency.
| padding-left: 0; | ||
| padding-right: 0; | ||
| margin-bottom: 1em; | ||
| padding-bottom: 1em; |
No description provided.