Layout change #70

pietrop · 2019-01-04T11:01:52Z

Is your Pull Request request related to another issue in this repository ?

Suggested Layout change #50

TL;DR:

Increasing the height of the space for text
allowing to have video preview both resizable and toggle show/hide option.

Describe what the PR does

changed layout
moved VideoPlayer preview into TranscriptEditor next to TimedTextEditor
added optional transcript title (non editable) with css logic to truncate if text is too long
added options in settings to hide preview
added option in settings to make video preview larger or smaller within custom range

State whether the PR is ready for review or whether it needs extra work

Still doing some tweaks, and cleaning up, should be ready for review soon.

Outstanding

Some of the logic across components might need refactoring and cleaning up - simplifying
The controls with all the playerControls btns could do with some spacing, to group by logic eg adding a space after fast forward, and after playback speed, before volume, and before title. (?)
Update QA doc with new functionalities (QA PR needs to merged into master before this one)
check that analytics events are still triggered (from QA doc list) - there might be one or two that are broken

Additional context

to accomodate for two new fields

leaving rest for another branch/PR tbc

pietrop · 2019-02-02T19:33:58Z

Outstanding:

rebase with current master
Needs adjusting spacing of elements in player controls (perhaps even order, to be more aesthetically pleasing)
consider moving How does this work? next to settings and keyboard shortcuts for space efficiency, see this previous PR help-box-spike: moved ToolTip next to settings icon #66
Would need QA on final version
in Select.modules.css - selectPlayerControl had to put width: 60px!important; to get the number in playback rate selection to show, there might be a better way to fix that css (?)
Tooltip says save with icon 💾 and it mentions export - to be addressed perhaps as separate PR

…or into layout-change

…r into layout-change

This reverts commit 2c6a9dd.

removed volume component and added titles tags to player controls btns

mobile break point

…Editor

pietrop · 2019-02-19T18:02:10Z

First pass at making it gracefully adapt for mobile view. #49

"How does it work" - mobile view
"settings" tooltip/modal - mobile view
"Keyboard shortcuts" tooltip/modal - mobile view
Timed text editor content is responsive: speakers, time-codes and text,

pietrop · 2019-02-20T13:22:28Z

Added an extra break point for ipad in portrait mode

It keeps the two columns for video and timed text editor, but inside the timed text editor rearranges speaker names and time-codes to be slightly more compact (same as in mobile view)

there's room for improvement

pietrop · 2019-02-20T21:24:54Z

needs removing hr tag that used to be under title and player controls, I think it go misplaced somewhere else but can be seen before loading the component (load demo in localhost)

jamesdools

I'm going to reject this for now - the styling is not quite there. Probably will have time to help out after the project cycle!

Might need a bit more UX input, especially while the rest of the project is changing too. I suggest we park this or tidy this up completely before it gets into master.

jamesdools · 2019-02-21T13:45:06Z

src/lib/TranscriptEditor/MediaPlayer/PlayerControls.js

+
+        <div className={ style.btnsGroup }>
+          <button
+            title="seek backward by a set interval"


Good to have these titles I think - but some uniformity would be good (text-wise)

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.

jamesdools · 2019-02-21T13:50:27Z

src/lib/TranscriptEditor/MediaPlayer/PlayerControls.js

-          title="Picture-in-picture"
-          className={ style.playerButton }
-          onClick={ this.props.pictureInPicture }
-        >


The formatting for closing elements > on the last line of a property / new line seems to change a lot

yeah, preference? should it just be up on the previous line?

jamesdools · 2019-02-21T13:51:11Z

src/lib/TranscriptEditor/MediaPlayer/PlayerControls.module.css

@@ -1,8 +1,12 @@
 @value color-darkest-grey, color-light-grey, color-labs-red from '../colours.module.css';

 .playerControls {
-  margin: 1em;
-  display: flex;
+  /* margin: 1em; */


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!

jamesdools · 2019-02-21T13:52:42Z

src/lib/TranscriptEditor/MediaPlayer/VideoPlayer.js

@@ -2,7 +2,23 @@ import React from 'react';
 import PropTypes from 'prop-types';

 class VideoPlayer extends React.Component {
+  constructor(props) {


useless constructor

jamesdools · 2019-02-21T13:53:08Z

src/lib/TranscriptEditor/MediaPlayer/VideoPlayer.js

+  }
+
+  handlePlayMedia = () => {
+    console.log('handle media', this.props.videoRef.current, this.props.videoRef);


tidy up logs

jamesdools · 2019-02-21T14:28:57Z

src/lib/TranscriptEditor/TimedTextEditor/WrapperBlock.module.css

+  .speaker {
+    display: inline-block;
+    width: 50%;
+    padding-right: 2em;


This offsets everything onto a new line:

This was quite tricky to get right the first time (even though it wasn't perfect), and so i'm reluctant to merge in these changes. In addition to the iphone / ipad view, the middle-sizes on desktop should be checked too.

There's a lot of space around the video player, where it could either take up the aside width or give more room to the editor

Yes, I think these needs to be addressed when we look at the media query breakpoints for the responsive design. So that as you said we can tweak for each size the various un-utilised spaces etc..

jamesdools · 2019-02-21T14:34:45Z

src/lib/TranscriptEditor/TimedTextEditor/WrapperBlock.module.css

-  text-overflow: ellipsis;
-  overflow: hidden;
-  white-space: nowrap;
+/* Mobile devices */


The WrapperBlock part looks good for the mobile widths 👌
I would maybe give the html/body a min-width of 320px so it doesn't squash down anymore.

not sure I follow?

jamesdools · 2019-02-21T14:35:16Z

src/lib/TranscriptEditor/TimedTextEditor/WrapperBlock.module.css

+/* Mobile devices */
+@media (max-width: 768px) {
+
+  .markers {


In mobile view, the speaker labels and timecodes are just a bit too close to the text. adding a margin-bottom would help

ok changed to be

.markers { ... margin-bottom: 0.5em;

and

.WrapperBlock { ... margin-bottom: 1em;

see what you think, we can tweak this

jamesdools · 2019-02-21T14:46:29Z

src/lib/TranscriptEditor/MediaPlayer/PlayerControls.module.css

@@ -1,8 +1,12 @@
 @value color-darkest-grey, color-light-grey, color-labs-red from '../colours.module.css';

 .playerControls {


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?

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.

jamesdools · 2019-02-21T14:48:34Z

src/lib/TranscriptEditor/Settings/index.js

+          <section className={ style.settingElement }>
+            <div className={ style.label }>Display Preview</div>
+            <Toggle
+              defaultValue={ this.props.previewIsDisplayed }


PropTypes missing for a lot of things on this branch. About 30+ things to tidy up on npm run lint too

ok fixed them all.

/src/lib/TranscriptEditor/index.js 284:18 warning Arrow function should not return assignment no-return-assign 285:23 warning Arrow function should not return assignment no-return-assign 286:23 warning Arrow function should not return assignment no-return-assign

these refer to this:

<MediaPlayer ... hookSeek={ foo => this.setCurrentTime = foo } hookPlayMedia={ foo => this.playMedia = foo } hookIsPlaying={ foo => this.isPlaying = foo }

I have to admit I don't fully understand how this works, and what could be an alternative way to do it (from a syntax point of view) I think this patter was originally introduced by @Laurian (?)

eg linting, css etc.. from James' PR feedback

ivanji · 2019-03-13T11:02:15Z

Could I contribute my changes? Please see attached. I'm using CSS grid instead and responsiveness it's pretty much taken care of. I still want to take care of some alignment issues, but happy to take on this issue instead and let @pietrop attend to more critical ones.

pietrop · 2019-03-13T14:36:54Z

Hi @ivanji , That's awesome, thanks!
Feel free to do a PR, it be great to see an alternative approach and a simpler way to tackle this.

One of the things to keep in mind is making sure the component can still work when used into a parent component that might have a different CSS setup (still using css modules tho - for now) if that makes sense? But we can always test that later.

pietrop · 2019-03-15T13:47:01Z

closing in favour of #111

Pietro Passarelli - News Labs added 8 commits January 3, 2019 19:43

moved ToolTip next to settings icon

2fac517

layout change refactor

8e365d1

added option to hide and resize preview window in settings

461d2bb

adjusted settings panel height

cf9fbca

to accomodate for two new fields

initial setup for mobile responsive

688cd28

leaving rest for another branch/PR tbc

Removed separate style file for video element

9790671

made fast ward and rewind btn accessible with tab and enter

57458ec

cleaned up some comments

e9b3245

pietrop added CSS Something to do with styling Design Something to do with Design labels Jan 4, 2019

reabsed with master

69b1e24

pietrop force-pushed the layout-change branch from 9bb5541 to 69b1e24 Compare February 2, 2019 19:31

Merge branch 'help-box-spike' of github.com:bbc/react-transcript-edit…

eb53feb

…or into layout-change

pietrop mentioned this pull request Feb 2, 2019

Suggested Layout change #50

Closed

Pietro Passarelli - News Labs added 2 commits February 2, 2019 15:33

Adding some css to player control

89a06ad

adjusted Select

ee20146

pietrop force-pushed the layout-change branch from f0bd8ad to ee20146 Compare February 19, 2019 11:58

pietrop and others added 10 commits February 19, 2019 14:57

Merge branch 'master' into layout-change

6e53d73

UI Tweaks

2c6a9dd

Merge branch 'layout-change' of github.com:bbc/react-transcript-edito…

4e10bbd

…r into layout-change

Revert "UI Tweaks"

9c05aae

This reverts commit 2c6a9dd.

player control fix

ccbe700

fixed unsuccesful merge

8f45a2e

moved title outside of player control

b4ab381

Removed Volume

37de921

removed volume component and added titles tags to player controls btns

first pass at making it responsive

8af3b78

mobile break point

first pass at making speakers and timecodes responsive in Timed Text …

5233619

…Editor

fixed CSS for ipad 🤞

a4f0ee7

responsive text in timed text editor for ipad and mobile view

b42d1c2

jamesdools suggested changes Feb 21, 2019

View reviewed changes

Pietro Passarelli - News Labs added 3 commits February 22, 2019 13:32

cleared out filename title overlap + updated README

9b1e659

mend

61f8d0c

various fixes

b37d1d5

eg linting, css etc.. from James' PR feedback

pietrop mentioned this pull request Mar 5, 2019

Mobile layout in parent component #107

Closed

ivanji mentioned this pull request Mar 14, 2019

Layout changes accessibility #111

Merged

pietrop closed this Mar 15, 2019

pietrop deleted the layout-change branch March 15, 2019 14:41

pietrop restored the layout-change branch March 18, 2019 15:39

pietrop deleted the layout-change branch March 18, 2019 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layout change #70

Layout change #70

pietrop commented Jan 4, 2019 •

edited

Loading

pietrop commented Feb 2, 2019 •

edited

Loading

pietrop commented Feb 19, 2019

pietrop commented Feb 20, 2019 •

edited

Loading

pietrop commented Feb 20, 2019

jamesdools left a comment

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

jamesdools Feb 21, 2019

jamesdools Feb 21, 2019

jamesdools Feb 21, 2019

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

pietrop Feb 28, 2019

jamesdools Feb 21, 2019

pietrop Mar 1, 2019 •

edited

Loading

ivanji commented Mar 13, 2019

pietrop commented Mar 13, 2019 •

edited

Loading

pietrop commented Mar 15, 2019

		@@ -1,8 +1,12 @@
		@value color-darkest-grey, color-light-grey, color-labs-red from '../colours.module.css';

		.playerControls {

Layout change #70

Layout change #70

Conversation

pietrop commented Jan 4, 2019 • edited Loading

pietrop commented Feb 2, 2019 • edited Loading

pietrop commented Feb 19, 2019

pietrop commented Feb 20, 2019 • edited Loading

pietrop commented Feb 20, 2019

jamesdools left a 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

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

pietrop Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

ivanji commented Mar 13, 2019

pietrop commented Mar 13, 2019 • edited Loading

pietrop commented Mar 15, 2019

pietrop commented Jan 4, 2019 •

edited

Loading

pietrop commented Feb 2, 2019 •

edited

Loading

pietrop commented Feb 20, 2019 •

edited

Loading

pietrop Mar 1, 2019 •

edited

Loading

pietrop commented Mar 13, 2019 •

edited

Loading