-
Notifications
You must be signed in to change notification settings - Fork 167
Layout change #70
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
Layout change #70
Changes from 24 commits
2fac517
8e365d1
461d2bb
cf9fbca
688cd28
9790671
57458ec
e9b3245
69b1e24
eb53feb
89a06ad
ee20146
6e53d73
2c6a9dd
4e10bbd
9c05aae
ccbe700
8f45a2e
b4ab381
37de921
8af3b78
5233619
a4f0ee7
b42d1c2
9b1e659
61f8d0c
b37d1d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
body { | ||
margin: 0px; | ||
} | ||
|
||
.container { | ||
height: 100vh; | ||
font-family: ReithSerif, Fallback, sans-serif; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import VolumeControl from './VolumeControl'; | ||
import Select from './Select'; | ||
|
||
import style from './PlayerControls.module.css'; | ||
|
@@ -23,14 +22,14 @@ class PlayerControls extends React.Component { | |
// backward or forward function | ||
// on mouseUp the interval is cleared | ||
setIntervalHelperBackward = () => { | ||
this.props.skipBackward(); | ||
// this.props.skipBackward(); | ||
this.interval = setInterval(() => { | ||
this.props.skipBackward(); | ||
}, 300); | ||
} | ||
|
||
setIntervalHelperForward = () => { | ||
this.props.skipForward(); | ||
// this.props.skipForward(); | ||
this.interval = setInterval(() => { | ||
this.props.skipForward(); | ||
}, 300); | ||
|
@@ -43,78 +42,87 @@ class PlayerControls extends React.Component { | |
render() { | ||
return ( | ||
<div className={ style.playerControls }> | ||
<button | ||
title="Rollback" | ||
className={ style.playerButton } | ||
onClick={ this.props.rollback }> | ||
<FontAwesomeIcon icon={ faUndo } /> | ||
</button> | ||
|
||
<button | ||
title="Rewind" | ||
className={ style.playerButton } | ||
onMouseDown={ this.setIntervalHelperBackward } | ||
onMouseUp={ this.clearIntervalHelper }> | ||
<FontAwesomeIcon icon={ faBackward } /> | ||
|
||
</button> | ||
|
||
<button | ||
title="Play" | ||
className={ style.playerButton } | ||
onClick={ this.props.playMedia }> | ||
{this.props.isPlaying ? <FontAwesomeIcon icon={ faPause } /> : <FontAwesomeIcon icon={ faPlay } />} | ||
</button> | ||
|
||
<button | ||
title="Forward" | ||
className={ style.playerButton } | ||
onMouseDown={ this.setIntervalHelperForward } | ||
onMouseUp={ this.clearIntervalHelper }> | ||
<FontAwesomeIcon icon={ faForward } /> | ||
</button> | ||
|
||
<span className={ style.playBackRate }> | ||
<Select | ||
title="Playback rate" | ||
options={ this.props.playbackRateOptions } | ||
currentValue={ this.props.playbackRate.toString() } | ||
name={ 'playbackRate' } | ||
handleChange={ this.props.setPlayBackRate } /> | ||
</span> | ||
|
||
<div className={ style.timeBox }> | ||
<span title="Current time" className={ style.currentTime } | ||
<span | ||
title="Current time" | ||
className={ style.currentTime } | ||
onClick={ this.props.promptSetCurrentTime } | ||
>{ this.props.currentTime }</span> | ||
<span className={ style.separator }>|</span> | ||
<span title="Clip duration" className={ style.duration }>{this.props.duration}</span> | ||
<span | ||
title="Clip duration" | ||
className={ style.duration } | ||
>{this.props.duration}</span> | ||
</div> | ||
|
||
<div className={ style.btnsGroup }> | ||
<button | ||
title="seek backward by a set interval" | ||
className={ style.playerButton } | ||
onClick={ this.props.rollback }> | ||
<FontAwesomeIcon icon={ faUndo } /> | ||
</button> | ||
|
||
<button | ||
title="seek backward" | ||
className={ style.playerButton } | ||
onMouseDown={ this.setIntervalHelperBackward } | ||
onMouseUp={ this.clearIntervalHelper } | ||
onClick={ () => {this.props.skipBackward(); } }> | ||
<FontAwesomeIcon icon={ faBackward } /> | ||
</button> | ||
|
||
<button | ||
title="Play/Pause" | ||
className={ style.playerButton } | ||
onClick={ this.props.playMedia }> | ||
{this.props.isPlaying ? <FontAwesomeIcon icon={ faPause } /> : <FontAwesomeIcon icon={ faPlay } />} | ||
</button> | ||
|
||
<button | ||
title="seek forward" | ||
className={ style.playerButton } | ||
onMouseDown={ this.setIntervalHelperForward } | ||
onMouseUp={ this.clearIntervalHelper } | ||
onClick={ () => {this.props.skipForward(); } }> | ||
<FontAwesomeIcon icon={ faForward } /> | ||
</button> | ||
</div> | ||
|
||
<button | ||
title="Save" | ||
className={ style.playerButton } | ||
onClick={ this.props.handleSaveTranscript }> | ||
<FontAwesomeIcon icon={ faSave } /> | ||
</button> | ||
|
||
<button | ||
title="Picture-in-picture" | ||
className={ style.playerButton } | ||
onClick={ this.props.pictureInPicture } | ||
> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The formatting for closing elements There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, preference? should it just be up on the previous line? |
||
<FontAwesomeIcon icon={ faTv } /> | ||
</button> | ||
|
||
<VolumeControl | ||
handleMuteVolume={ this.props.handleMuteVolume } | ||
/> | ||
<div className={ style.btnsGroup }> | ||
<span className={ style.playBackRate } | ||
title="Playback rate" | ||
> | ||
<Select | ||
options={ this.props.playbackRateOptions } | ||
currentValue={ this.props.playbackRate.toString() } | ||
name={ 'playbackRate' } | ||
handleChange={ this.props.setPlayBackRate } /> | ||
</span> | ||
|
||
<button | ||
title="Save" | ||
className={ style.playerButton } | ||
onClick={ this.props.handleSaveTranscript }> | ||
<FontAwesomeIcon icon={ faSave } /> | ||
</button> | ||
|
||
<button | ||
title="Picture-in-picture" | ||
className={ style.playerButton } | ||
onClick={ this.props.pictureInPicture } | ||
> | ||
<FontAwesomeIcon icon={ faTv } /> | ||
</button> | ||
|
||
</div> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
PlayerControls.propTypes = { | ||
|
||
playMedia: PropTypes.func, | ||
currentTime: PropTypes.string, | ||
timecodeOffset: PropTypes.string, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
@value color-darkest-grey, color-light-grey, color-labs-red from '../colours.module.css'; | ||
|
||
.playerControls { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section needs rethinking. The player controls and the grouped buttons within could probably be centred and responsive to narrow screens with flexbox and + wrap? Not sure how fiddly that'll be. I find here the Timebox is too far away from the player + progress bar. Also the huge grid of buttons is overwhelming. An alternative for mobile could be player controls + timebox stuck to the bottom of the screen? Probably needs more UX input rather than winging it. We're undoing a lot of stuff from the first round of UX in this PR so let's not get carried away 😛 Also, any idea why the MediaPlayer size doesn't update properly a lot of the time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can rethink it, I reckon we can just do a first pass in this PR and then optimise later. Not good to try and do everything at once. |
||
margin: 1em; | ||
display: flex; | ||
/* margin: 1em; */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets get rid of as much CSS half-fixes and comments as possible - it gets confusing if you're not the one who wrote it! |
||
/* margin-top: 0.5em; */ | ||
margin-left: 1em; | ||
/* display: flex; */ | ||
/* justify-content: flex-start; */ | ||
|
||
} | ||
|
||
.playerControls > * { | ||
|
@@ -11,7 +15,7 @@ | |
|
||
.playerControls > *:not(:last-child) { | ||
margin-right: 0.5em; | ||
background: color-darkest-grey; | ||
/* background: color-darkest-grey; */ | ||
} | ||
|
||
.playerButton { | ||
|
@@ -20,24 +24,35 @@ | |
padding: 0.5em; | ||
border: 0; | ||
color: white; | ||
background: color-darkest-grey; | ||
font-size: 1em; | ||
cursor: pointer; | ||
/* border-color: black; | ||
border-width: 0.1rem; | ||
border-style: solid; */ | ||
|
||
margin-right: 0.3rem; | ||
margin-top: 0.3rem; | ||
} | ||
|
||
.playBackRate{ | ||
height: 48px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this does anything here? It's good to make sure each rule is doing what you think it's doing, or it'll turn into a headache afterwards 😱 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, well spotted, removed |
||
width: 70px; | ||
border: 0; | ||
color: white; | ||
/* background: color-darkest-grey; */ | ||
font-size: 1em; | ||
cursor: pointer; | ||
position: relative; | ||
padding-left: 0.8em; | ||
/* padding-left: 0.8em; */ | ||
margin-right: 0.3rem; | ||
} | ||
|
||
.playBackRate::before{ | ||
content: '×'; | ||
position: absolute; | ||
bottom: 11px; | ||
left: 12px; | ||
bottom: -2px; | ||
left: 21px;/*<-- */ | ||
} | ||
|
||
.playBackRate > select { | ||
|
@@ -47,13 +62,16 @@ | |
outline: none; | ||
width: auto; | ||
width: 100%; | ||
color:white; | ||
background-color: color-darkest-grey; | ||
} | ||
|
||
.timeBox { | ||
display: inline-block; | ||
text-align: center; | ||
line-height: 48px; | ||
padding: 0 1em; | ||
background-color: color-darkest-grey; | ||
} | ||
|
||
.currentTime { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to have these titles I think - but some uniformity would be good (text-wise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair, we had some feedback from UX about research they have been done on naming, I think this should be noted down for our next batch of user testing follow up questions, to ask how they would refer to the various functionalities, for naming.