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.

Sign up for GitHub

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

fix: video editor and uploader layout fixes #410

Merged
merged 5 commits into from
Dec 4, 2023
Merged

fix: video editor and uploader layout fixes #410

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b1ca6fe
fix: video upload page layout fixes
ArturGaspar Oct 17, 2023
212578c
fix: video editor thumbnail fallback icon colour and size
ArturGaspar Oct 17, 2023
85cc5be
fix: video uploader close button go back instead of closing app
ArturGaspar Oct 18, 2023
204a70f
fix: video editor spinners vertical alignment
ArturGaspar Oct 19, 2023
67a53d0
fix: use trailingElement for video uploader input button
ArturGaspar Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/editors/Editor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,19 @@ export const Editor = ({

const EditorComponent = supportedEditors[blockType];
return (
<div className="d-flex flex-column">
<div
className="d-flex flex-column"
style={{
/* Positioned as a proper Paragon FullscreenModal should have been. */
position: 'fixed',
top: 0,
left: 0,
right: 0,
height: '100%',
Comment on lines +38 to +42
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

}}
>
<div
className="pgn__modal-fullscreen"
className="pgn__modal-fullscreen h-100"
role="dialog"
aria-label={blockType}
>
Expand Down
22 changes: 20 additions & 2 deletions src/editors/__snapshots__/Editor.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,19 @@
exports[`Editor render presents error message if no relevant editor found and ref ready 1`] = `
<div
className="d-flex flex-column"
style={
Object {
"height": "100%",
"left": 0,
"position": "fixed",
"right": 0,
"top": 0,
}
}
>
<div
aria-label="fAkEBlock"
className="pgn__modal-fullscreen"
className="pgn__modal-fullscreen h-100"
role="dialog"
>
<FormattedMessage
Expand All @@ -21,10 +30,19 @@ exports[`Editor render presents error message if no relevant editor found and re
exports[`Editor render snapshot: renders correct editor given blockType (html -> TextEditor) 1`] = `
<div
className="d-flex flex-column"
style={
Object {
"height": "100%",
"left": 0,
"position": "fixed",
"right": 0,
"top": 0,
}
}
>
<div
aria-label="html"
className="pgn__modal-fullscreen"
className="pgn__modal-fullscreen h-100"
role="dialog"
>
<TextEditor
Expand Down
18 changes: 14 additions & 4 deletions src/editors/containers/EditorContainer/__snapshots__/index.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@

exports[`EditorContainer component render snapshot: initialized. enable save and pass to header 1`] = `
<div
className="position-relative zindex-0"
className="d-flex flex-column position-relative zindex-0"
style={
Object {
"minHeight": "100%",
}
}
>
<BaseModal
bodyStyle={null}
Expand Down Expand Up @@ -53,7 +58,7 @@ exports[`EditorContainer component render snapshot: initialized. enable save and
</div>
</ModalDialog.Header>
<ModalDialog.Body
className="pb-6"
className="pb-0 mb-6"
>
<h1>
My test content
Expand All @@ -78,7 +83,12 @@ exports[`EditorContainer component render snapshot: initialized. enable save and

exports[`EditorContainer component render snapshot: not initialized. disable save and pass to header 1`] = `
<div
className="position-relative zindex-0"
className="d-flex flex-column position-relative zindex-0"
style={
Object {
"minHeight": "100%",
}
}
>
<BaseModal
bodyStyle={null}
Expand Down Expand Up @@ -129,7 +139,7 @@ exports[`EditorContainer component render snapshot: not initialized. disable sav
</div>
</ModalDialog.Header>
<ModalDialog.Body
className="pb-6"
className="pb-0 mb-6"
/>
<injectIntl(ShimmedIntlComponent)
disableSave={true}
Expand Down
5 changes: 3 additions & 2 deletions src/editors/containers/EditorContainer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ export const EditorContainer = ({
const handleCancel = hooks.handleCancel({ onClose, returnFunction });
return (
<div
className="position-relative zindex-0"
className="d-flex flex-column position-relative zindex-0"
style={{ minHeight: '100%' }}
>
<BaseModal
size="md"
Expand Down Expand Up @@ -64,7 +65,7 @@ export const EditorContainer = ({
/>
</div>
</ModalDialog.Header>
<ModalDialog.Body className="pb-6">
<ModalDialog.Body className="pb-0 mb-6">
{isInitialized && children}
</ModalDialog.Body>
<EditorFooter
Expand Down
2 changes: 1 addition & 1 deletion ...tainers/VideoEditor/components/VideoSettingsModal/components/VideoPreviewWidget/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const VideoPreviewWidget = ({
<div className="d-flex flex-row">
<Image
thumbnail
className="mr-3 p-4"
className="mr-3 px-6 py-4.5"
ref={imgRef}
src={thumbnailImage}
alt={intl.formatMessage(thumbnailMessages.thumbnailAltText)}
Expand Down
10 changes: 7 additions & 3 deletions src/editors/containers/VideoGallery/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,14 @@ export const useVideoListProps = ({
};
};

export const useVideoUploadHandler = () => {
export const useVideoUploadHandler = ({ replace }) => {
const learningContextId = useSelector(selectors.app.learningContextId);
const blockId = useSelector(selectors.app.blockId);
return () => navigateTo(`/course/${learningContextId}/editor/video_upload/${blockId}`);
const path = `/course/${learningContextId}/editor/video_upload/${blockId}`;
if (replace) {
return () => window.location.replace(path);
}
return () => navigateTo(path);
};

export const useCancelHandler = () => (
Expand Down Expand Up @@ -203,7 +207,7 @@ export const useVideoProps = ({ videos }) => {
inputError,
selectBtnProps,
} = videoList;
const fileInput = { click: useVideoUploadHandler() };
const fileInput = { click: useVideoUploadHandler({ replace: false }) };

return {
galleryError,
Expand Down
2 changes: 1 addition & 1 deletion src/editors/containers/VideoGallery/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export const VideoGallery = () => {
(state) => selectors.requests.isFailed(state, { requestKey: RequestKeys.uploadVideo }),
);
const videos = hooks.buildVideos({ rawVideos });
const handleVideoUpload = hooks.useVideoUploadHandler();
const handleVideoUpload = hooks.useVideoUploadHandler({ replace: true });

useEffect(() => {
// If no videos exists redirects to the video upload screen
Expand Down
6 changes: 3 additions & 3 deletions src/editors/containers/VideoGallery/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('VideoGallery', () => {
beforeAll(() => {
oldLocation = window.location;
delete window.location;
window.location = { assign: jest.fn() };
window.location = { replace: jest.fn() };
});
afterAll(() => {
window.location = oldLocation;
Expand Down Expand Up @@ -118,10 +118,10 @@ describe('VideoGallery', () => {
));
});
it('navigates to video upload page when there are no videos', async () => {
expect(window.location.assign).not.toHaveBeenCalled();
expect(window.location.replace).not.toHaveBeenCalled();
updateState({ videos: [] });
await renderComponent();
expect(window.location.assign).toHaveBeenCalled();
expect(window.location.replace).toHaveBeenCalled();
});
it.each([
[/by date added \(newest\)/i, [2, 1, 3]],
Expand Down
56 changes: 28 additions & 28 deletions src/editors/containers/VideoUploadEditor/VideoUploader.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { ArrowForward, FileUpload, Close } from '@edx/paragon/icons';
import { useDispatch } from 'react-redux';
import { thunkActions } from '../../data/redux';
import * as hooks from './hooks';
import * as editorHooks from '../EditorContainer/hooks';
import messages from './messages';

const URLUploader = () => {
Expand All @@ -17,49 +16,51 @@ const URLUploader = () => {
const intl = useIntl();
return (
<div className="d-flex flex-column">
<div style={{ backgroundColor: '#F2F0EF' }} className="justify-content-center align-self-center rounded-circle p-5">
<Icon src={FileUpload} className="text-muted" size="lg" />
<div className="justify-content-center align-self-center rounded-circle bg-light-300 p-2.5">
<Icon src={FileUpload} className="text-muted" style={{ height: '2rem', width: '2rem' }} />
</div>
<div className="d-flex align-self-center justify-content-center flex-wrap flex-column pt-5">
<span className="small">{intl.formatMessage(messages.dropVideoFileHere)}</span>
<span className="align-self-center" style={{ fontSize: '0.8rem' }}>{intl.formatMessage(messages.info)}</span>
<div className="d-flex align-self-center justify-content-center flex-wrap flex-column pt-3">
<span>{intl.formatMessage(messages.dropVideoFileHere)}</span>
<span className="x-small align-self-center pt-2">{intl.formatMessage(messages.info)}</span>
</div>
<div className="x-small align-self-center justify-content-center mx-2 text-dark font-weight-normal">OR</div>
<div className="zindex-9 video-id-prompt p-4">
<InputGroup className="video-upload-input-group">
<div className="small align-self-center justify-content-center mx-2 text-dark font-weight-normal pt-3">
OR
</div>
<div className="zindex-9 video-id-prompt py-3">
<InputGroup>
<FormControl
className="m-0"
Copy link
Member

Choose a reason for hiding this comment

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

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

Screenshot 2023-11-30 at 3 38 01 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KristinAoki Using the trailingElement fixed this.

placeholder={intl.formatMessage(messages.pasteURL)}
aria-label={intl.formatMessage(messages.pasteURL)}
aria-describedby="basic-addon2"
borderless
onClick={(event) => { event.stopPropagation(); }}
onChange={(event) => { setTextInputValue(event.target.value); }}
/>
<div className="light-300 justify-content-center align-self-center bg-light rounded-circle p-0 x-small url-submit-button">
<IconButton
className="text-muted"
alt={intl.formatMessage(messages.submitButtonAltText)}
src={ArrowForward}
iconAs={Icon}
size="inline"
onClick={(event) => {
event.stopPropagation();
if (textInputValue.trim() !== '') {
onURLUpload(textInputValue);
}
}}
/>
</div>
<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);
}
}}
/>
Copy link
Member

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KristinAoki Done.

</InputGroup>
</div>
</div>
);
};

export const VideoUploader = ({ setLoading, onClose }) => {
export const VideoUploader = ({ setLoading }) => {
const dispatch = useDispatch();
const intl = useIntl();
const handleCancel = editorHooks.handleCancel({ onClose });
const goBack = hooks.useHistoryGoBack();

const handleProcessUpload = ({ fileData }) => {
dispatch(thunkActions.video.uploadVideo({
Expand All @@ -77,7 +78,7 @@ export const VideoUploader = ({ setLoading, onClose }) => {
alt={intl.formatMessage(messages.closeButtonAltText)}
src={Close}
iconAs={Icon}
onClick={handleCancel}
onClick={goBack}
/>
</div>
<Dropzone
Expand All @@ -91,7 +92,6 @@ export const VideoUploader = ({ setLoading, onClose }) => {

VideoUploader.propTypes = {
setLoading: PropTypes.func.isRequired,
onClose: PropTypes.func.isRequired,
};

export default VideoUploader;
Loading
Loading