fix: video editor and uploader layout fixes #410

ArturGaspar · 2023-10-17T14:54:43Z

Description

Fix video editor/upload layout.

Video editor spinner before/after:
Video editor thumbnail before/after:
Upload page before/after:
Upload spinner before/after:

Testing instructions

Layout changes

(Optional) Create a mock video upload API with a useful response delay

Enable mock video uploads (create waffle flag contentstore.mock_video_uploads enabled for everyone)

Modify videos_post() as follows:

def videos_post(course, request):
    """
    ...
    """

    # Delay for observing spinner.
    import time
    time.sleep(20)

    if use_mock_video_uploads():
        return {'files': [{
            'file_name': 'video.mp4', 'upload_url': 'http://example.com/put_video', 'edx_video_id': '1234'
        }]}, 200

    ...

Open video editor
1. Enable the new video editor (create waffle flags new_core_editors.use_new_video_editor and new_core_editors.use_video_gallery_flow enabled for everyone)
2. Create a course unit with a video
3. Edit the video (should open in new editor)
See that loading spinner is vertically centered
See that video editor thumbnail icon is grey and smaller
Open video uploader
1. Click "< Replace video" to go to the gallery
2. If there are videos in the gallery and the uploader does not open automatically, click "+ Upload or embed a new video"
See the layout changes
Upload a video
See that loading spinner is vertically centered

Video uploader close button

(Optional) Create a mock video API with useful responses

Enable mock video uploads (create waffle flag contentstore.mock_video_uploads enabled for everyone)

Modify videos_index_json() as follows:

def videos_index_json(course):
    """
    ...
    """
    # index_videos, __ = _get_index_videos(course)
    index_videos = [
        {
            'edx_video_id': 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa',
            'client_video_id': 'video.mp4',
            'created': '1970-01-01T00:00:00Z',
            'duration': 42.5,
            'status': 'upload',
            'course_video_image_url': 'https://video/images/1234.jpg'
        },
        {
            'edx_video_id': 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa',
            'client_video_id': 'video.mp4',
            'created': '1970-01-01T00:00:00Z',
            'duration': 42.5,
            'status': 'upload',
            'course_video_image_url': 'https://video/images/1234.jpg'
        }
    ]
    return JsonResponse({"videos": index_videos}, status=200)

Enable the new video editor (create waffle flags new_core_editors.use_new_video_editor and new_core_editors.use_video_gallery_flow enabled for everyone)
Create a course unit with a video
Open video gallery
1. Click "Edit" on the video from step 3 (should open in new editor)
2. Click "< Replace video" to go to the gallery
Make sure the uploader is not automatically loaded
Click "+ Upload or embed a new video"
Click the close (X) button
Check that you are taken back to the gallery

Modify the function from step 1 to return an empty list, wait for the application to properly reload:

def videos_index_json(course):
    """
    ...
    """
    # index_videos, __ = _get_index_videos(course)
    index_videos = []
    return JsonResponse({"videos": index_videos}, status=200)

Open video gallery (repeat step 4)
Make sure the uploader is automatically loaded after the gallery
Click the close (X) button
Check that you are taken back to the video editing page

openedx-webhooks · 2023-10-17T14:54:48Z

Thanks for the pull request, @ArturGaspar! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-10-17T18:53:56Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (cfa4577) 90.49% compared to head (67a53d0) 90.53%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/editors/containers/VideoUploadEditor/index.jsx	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
+ Coverage   90.49%   90.53%   +0.03%     
==========================================
  Files         228      227       -1     
  Lines        4115     4111       -4     
  Branches      822      826       +4     
==========================================
- Hits         3724     3722       -2     
+ Misses        370      369       -1     
+ Partials       21       20       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Change the video uploader close button to always go back in navigation history, and change the gallery to replace its location with the video uploader's when automatically loading it due to an empty video list, so that when the uploader goes back in the history, it goes to what was before the gallery.

ArturGaspar · 2023-10-24T16:39:52Z

The coverage loss is only a move of uncovered line 24 to line 17.

Mocking the VideoUploader component and calling the setLoading argument that VideoUploadEditor passes to it should cover it, but I could not figure out how to mock it.

farhaanbukhsh · 2023-10-26T10:22:52Z

src/editors/Editor.jsx

+        position: 'fixed',
+        top: 0,
+        left: 0,
+        right: 0,
+        height: '100%',


Using CSS classes seems to be better here https://paragon-openedx.netlify.app/foundations/css-utilities

@farhaanbukhsh There is no CSS class for top: 0, left: 0 or right: 0, and I thought leaving everything under style makes it clearer that this entire part is copied from the styling of the Paragon FullscreenModal.

farhaanbukhsh

👍

✅ I tested this on devstack and it looks good apart from the one comment I have
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

The Editor component uses the pgn__modal-fullscreen class to be fullscreen, but it is not structured like a Paragon FullscreenModal and the fullscreen positioning style is not applied correctly, particularly in the case where the content is smaller than the body - a vertically centred component will be centred to the content size, not to the screen. Here we directly apply the style that would have applied to the relevant elements of a Paragon FullscreenModal.

tecoholic · 2023-11-09T10:21:33Z

@navinkarkera Can you kindly add the "jira:2u" label to this PR?

cc: @cablaa77

e0d · 2023-11-20T18:05:47Z

@tecoholic is this blended work?

mphilbrick211 · 2023-11-20T18:27:00Z

Hi @openedx/teaching-and-learning - this is ready for review!

openedx-webhooks · 2023-11-20T18:53:39Z

I've created issue TNL-11223 in the private 2U Jira.

tecoholic · 2023-11-21T07:37:40Z

@e0d Yes. This is part of the Video Block UX Improvements work.

KristinAoki · 2023-11-30T20:41:29Z

src/editors/containers/VideoUploadEditor/VideoUploader.jsx

          <FormControl
+            className="m-0"


When the link for a video is extends the full width of the input box, the submit arrow covers the text.

@KristinAoki Using the trailingElement fixed this.

KristinAoki · 2023-11-30T20:45:53Z

src/editors/containers/VideoUploadEditor/VideoUploader.jsx

+          <IconButton
+            className="position-absolute align-self-center text-muted bg-transparent shadow-none"
+            style={{ marginLeft: '20.25rem' }}
+            alt={intl.formatMessage(messages.submitButtonAltText)}
+            src={ArrowForward}
+            iconAs={Icon}
+            size="inline"
+            onClick={(event) => {
+              event.stopPropagation();
+              if (textInputValue.trim() !== '') {
+                onURLUpload(textInputValue);
+              }
+            }}
+          />


Is there a reason you are using IconButton to create the submit button instead of using the trailingElement attribute for FormControl? This would eliminate the extra classes and style configurations. For example see the EditableHeader component:

<Form.Control style={{ paddingInlineEnd: 'calc(1rem + 84px)' }} // extra style is because there are two buttons autoFocus trailingElement={<EditConfirmationButtons {...{ updateTitle, cancelEdit }} />} onChange={handleChange} onKeyDown={handleKeyDown} placeholder="Title" ref={inputRef} value={localTitle} />

where EditConfirmationButtons is

<ButtonGroup> <IconButtonWithTooltip tooltipPlacement="left" tooltipContent={intl.formatMessage(messages.saveTitleEdit)} src={Check} iconAs={Icon} onClick={updateTitle} /> <IconButtonWithTooltip tooltipPlacement="right" tooltipContent={intl.formatMessage(messages.cancelTitleEdit)} src={Close} iconAs={Icon} onClick={cancelEdit} /> </ButtonGroup>

@KristinAoki Done.

Also styles the button so it looks the same on hover/active.

openedx-webhooks · 2023-12-04T14:58:21Z

@ArturGaspar 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

jesperhodge · 2023-12-13T16:28:06Z

Good morning @ArturGaspar . I work for the Teaching and Learning team (TNL) at edX and have two questions for you, with context below. The first regards your intentions behind styling decisions you recently merged. The second regards whether you'd like to author a fix, or whether you'd prefer us to.

Regarding this PR, we ran into a problem yesterday that called for immediate attention, as partners working on exam problems were blocked by the absence of vertical scroll bars.
In looking at the PR and changes to Editor.jsx, specifically, you had an exchange with @farhaanbukhsh in which you appear to be leaning into the Paragon modal styling, in an attempt to be as standard as possible.
It appears to be here that the problem arises, with styling calling for fixed position and some related changes.
Could you please clarify for me your rationale behind changing CSS properties to position: fixed and height: 100%, and the code comment of "Positioned as a proper Paragon FullscreenModal should have been." What problem were you solving, and how is this the solution? I ask because your answer may affect what's an appropriate fix.

We reverted these changes yesterday, to restore problem editor functionality, and are now looking to clean up.

We'd like to have this problem addressed by the end of tomorrow. What's your preference? Would you like to be the one to fix the bug, or would you prefer us to? We are of course happy to review changes you propose if you choose to author the fix.

bszabo · 2023-12-14T15:55:46Z

@ArturGaspar Bernard Szabo here, also from TNL, following up on Jesper's comment from yesterday. The frontend-app-course-authoring reverts we put in place to buy time for fixing FLCC have caused a separate set of serious problems, and we urgently need to undo them. TNL is actively working today in the space of this PR to fix FLCC to enable us to bring back all frontend-app-course-authoring functionality ASAP.

ArturGaspar · 2023-12-15T12:11:31Z

@jesperhodge Hello. Apologies for the delay in responding. The reason for that decision was that, the component previously used the pgn__modal-fullscreen class, which caused some styilng for the Paragon FullscreenModal to be applied to it. However it did not have the entire inner structure of that modal, which caused the lading spinner to not be vertically centred.

I changed those CSS properties as they were what would have been applied to an inner part of a FullscreenModal, and they fixed the spinner vertical alignment.

ArturGaspar · 2023-12-15T12:42:02Z

@bszabo I see that there were changes to frontend-app-course-authoring as well as to flcc. Is there anything I can still help with?

jesperhodge · 2023-12-18T16:21:38Z

@ArturGaspar Hi Artur, we were able to create a fix to this that dealt with the most critical problems. I'd need to test whether the loading spinner is positioned correctly now - that was not our big priority due to the incident urgency.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 17, 2023

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch from 7cc28e4 to 6adf4e0 Compare October 17, 2023 15:20

mphilbrick211 added needs test run Author's first PR to this repository, awaiting test authorization from Axim and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Oct 17, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 17, 2023

ArturGaspar added 2 commits October 18, 2023 15:04

fix: video upload page layout fixes

b1ca6fe

fix: video editor thumbnail fallback icon colour and size

212578c

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch 2 times, most recently from ee907e3 to b637757 Compare October 18, 2023 20:27

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 19, 2023

ArturGaspar changed the title ~~fix: video upload page layout fixes~~ fix: video editor and uploader layout fixes Oct 19, 2023

ArturGaspar marked this pull request as ready for review October 19, 2023 17:18

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch from e376690 to 772a9c9 Compare October 23, 2023 14:52

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch from 772a9c9 to f550bf9 Compare October 23, 2023 14:57

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 24, 2023

farhaanbukhsh reviewed Oct 26, 2023

View reviewed changes

farhaanbukhsh approved these changes Oct 26, 2023

View reviewed changes

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch from f550bf9 to 204a70f Compare October 26, 2023 12:39

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 14, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 20, 2023

mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Nov 20, 2023

mphilbrick211 requested a review from a team November 20, 2023 18:27

jristau1984 added the jira:2u We want an issue in the 2U Jira instance label Nov 20, 2023

KristinAoki suggested changes Nov 30, 2023

View reviewed changes

fix: use trailingElement for video uploader input button

67a53d0

Also styles the button so it looks the same on hover/active.

ArturGaspar force-pushed the artur/video-upload-layout-fixes branch from f406f56 to 67a53d0 Compare December 4, 2023 12:32

KristinAoki approved these changes Dec 4, 2023

View reviewed changes

KristinAoki merged commit 1ddaf9a into openedx:main Dec 4, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: video editor and uploader layout fixes #410

fix: video editor and uploader layout fixes #410

ArturGaspar commented Oct 17, 2023 •

edited

Loading

openedx-webhooks commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

ArturGaspar commented Oct 24, 2023

farhaanbukhsh Oct 26, 2023

ArturGaspar Oct 26, 2023

farhaanbukhsh left a comment

tecoholic commented Nov 9, 2023 •

edited

Loading

e0d commented Nov 20, 2023

mphilbrick211 commented Nov 20, 2023

openedx-webhooks commented Nov 20, 2023

tecoholic commented Nov 21, 2023

KristinAoki Nov 30, 2023

ArturGaspar Dec 4, 2023

KristinAoki Nov 30, 2023

ArturGaspar Dec 4, 2023

openedx-webhooks commented Dec 4, 2023

jesperhodge commented Dec 13, 2023

bszabo commented Dec 14, 2023

ArturGaspar commented Dec 15, 2023

ArturGaspar commented Dec 15, 2023

jesperhodge commented Dec 18, 2023

fix: video editor and uploader layout fixes #410

fix: video editor and uploader layout fixes #410

Conversation

ArturGaspar commented Oct 17, 2023 • edited Loading

Description

Testing instructions

Layout changes

Video uploader close button

openedx-webhooks commented Oct 17, 2023 • edited Loading

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

ArturGaspar commented Oct 24, 2023

farhaanbukhsh Oct 26, 2023

Choose a reason for hiding this comment

ArturGaspar Oct 26, 2023

Choose a reason for hiding this comment

farhaanbukhsh left a comment

Choose a reason for hiding this comment

tecoholic commented Nov 9, 2023 • edited Loading

e0d commented Nov 20, 2023

mphilbrick211 commented Nov 20, 2023

openedx-webhooks commented Nov 20, 2023

tecoholic commented Nov 21, 2023

KristinAoki Nov 30, 2023

Choose a reason for hiding this comment

ArturGaspar Dec 4, 2023

Choose a reason for hiding this comment

KristinAoki Nov 30, 2023

Choose a reason for hiding this comment

ArturGaspar Dec 4, 2023

Choose a reason for hiding this comment

openedx-webhooks commented Dec 4, 2023

jesperhodge commented Dec 13, 2023

bszabo commented Dec 14, 2023

ArturGaspar commented Dec 15, 2023

ArturGaspar commented Dec 15, 2023

jesperhodge commented Dec 18, 2023

ArturGaspar commented Oct 17, 2023 •

edited

Loading

openedx-webhooks commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

tecoholic commented Nov 9, 2023 •

edited

Loading