Page MenuHomePhabricator
Differential D7899 Diff 26726 web/media/multimedia.react.js

Changeset View

Standalone View

View Options

web/media/multimedia.react.js

// @flow // @flow
import classNames from 'classnames'; import classNames from 'classnames';
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { CircularProgressbar } from 'react-circular-progressbar'; import { CircularProgressbar } from 'react-circular-progressbar';
import 'react-circular-progressbar/dist/styles.css'; import 'react-circular-progressbar/dist/styles.css';
import { import {
XCircle as XCircleIcon, XCircle as XCircleIcon,
AlertCircle as AlertCircleIcon, AlertCircle as AlertCircleIcon,
} from 'react-feather'; } from 'react-feather';
import { import { useModalContext } from 'lib/components/modal-provider.react.js';
useModalContext,
type PushModal,
} from 'lib/components/modal-provider.react.js';
import { fetchableMediaURI } from 'lib/media/media-utils.js'; import { fetchableMediaURI } from 'lib/media/media-utils.js';
import type { MediaType, EncryptedMediaType } from 'lib/types/media-types.js'; import type { MediaType, EncryptedMediaType } from 'lib/types/media-types.js';
import EncryptedMultimedia from './encrypted-multimedia.react.js'; import EncryptedMultimedia from './encrypted-multimedia.react.js';
import css from './media.css'; import css from './media.css';
import MultimediaModal from './multimedia-modal.react.js'; import MultimediaModal from './multimedia-modal.react.js';
import Button from '../components/button.react.js'; import Button from '../components/button.react.js';
import { type PendingMultimediaUpload } from '../input/input-state.js'; import { type PendingMultimediaUpload } from '../input/input-state.js';
type MediaSource = type MediaSource =
| { | {
+type: MediaType, +type: MediaType,
+uri: string, +uri: string,
} }
| { | {
+type: EncryptedMediaType, +type: EncryptedMediaType,
+holder: string, +holder: string,
+encryptionKey: string, +encryptionKey: string,
}; };
type BaseProps = { type Props = {
+mediaSource: MediaSource, +mediaSource: MediaSource,
+pendingUpload?: ?PendingMultimediaUpload, +pendingUpload?: ?PendingMultimediaUpload,
+remove?: (uploadID: string) => void, +remove?: (uploadID: string) => void,
+multimediaCSSClass: string, +multimediaCSSClass: string,
+multimediaImageCSSClass: string, +multimediaImageCSSClass: string,
}; };
type Props = {
...BaseProps,
+pushModal: PushModal,
};
class Multimedia extends React.PureComponent<Props> { function Multimedia(props: Props): React.Node {
componentDidUpdate(prevProps: Props) { const { mediaSource, pendingUpload } = props;
const { mediaSource, pendingUpload } = this.props; const prevPropsRef = React.useRef({ mediaSource, pendingUpload });
React.useEffect(() => {
const prevProps = prevPropsRef.current;
prevPropsRef.current = { mediaSource, pendingUpload };
if ( if (
prevProps.mediaSource.type === 'encrypted_photo' || prevProps.mediaSource.type === 'encrypted_photo' ||
prevProps.mediaSource.type === 'encrypted_video' prevProps.mediaSource.type === 'encrypted_video'
) { ) {
return; return;
} }
const prevUri = prevProps.mediaSource?.uri; const prevUri = prevProps.mediaSource.uri;
if (!prevUri || mediaSource.uri === prevUri) { if (!prevUri || mediaSource.uri === prevUri) {
return; return;
} }
if ( if (
(!pendingUpload || pendingUpload.uriIsReal) && (!pendingUpload || pendingUpload.uriIsReal) &&
(!prevProps.pendingUpload || !prevProps.pendingUpload.uriIsReal) (!prevProps.pendingUpload || !prevProps.pendingUpload.uriIsReal)
) { ) {
URL.revokeObjectURL(prevUri); URL.revokeObjectURL(prevUri);
} }
}, [mediaSource, pendingUpload]);
const { remove: removeProp } = props;
const handleRemove = React.useCallback(
(event: SyntheticEvent<HTMLElement>) => {
event.stopPropagation();
invariant(
removeProp && pendingUpload,
'Multimedia cannot be removed as either remove or pendingUpload ' +
'are unspecified',
);
removeProp(pendingUpload.localID);
},
[removeProp, pendingUpload],
);
const { pushModal } = useModalContext();
const handleClick = React.useCallback(() => {
let media;
if (
mediaSource.type === 'encrypted_photo' ||
mediaSource.type === 'encrypted_video'
) {
const { type, holder, encryptionKey } = mediaSource;
media = { type, holder, encryptionKey };
} else {
const { type, uri } = mediaSource;
invariant(uri, 'uri is missing for media modal');
ashoatUnsubmitted
Not Done
Inline Actions

Wondering why this invariant is necessary... based on Flow types, it appears that uri should always be set (but perhaps it can be an empty string?)

ashoat: Wondering why this `invariant` is necessary... based on Flow types, it appears that `uri`…
bartekAuthorUnsubmitted
Done
Inline Actions

Wondering why this invariant is necessary... based on Flow types, it appears that uri should always be set (but perhaps it can be an empty string?)

This invariant is for Flow which for some reason isn't able to see that we're already excluded EncryptedMediaType media sources, and for MediaType sources uri is always present.

Anyway, I update the MultimediaModal props to match mediaSource type again so this change will be reverted.

bartek: > Wondering why this `invariant` is necessary... based on Flow types, it appears that `uri`…
media = { type, uri };
} }
pushModal(<MultimediaModal media={media} />);
ashoatUnsubmitted
Not Done
Inline Actions

It appears that this code has changed... previously it was just passing mediaSource to media.

Two quick notes:

  1. Can you clarify why it changed? Was it a Flow issue?
  2. When possible, it's always best to separate changes from refactor diffs... that way it's easier for the reviewer to "blindly accept" the refactor, and then pay closer attention to the actual changes
ashoat: It appears that this code has changed... previously it was just passing `mediaSource` to…
bartekAuthorUnsubmitted
Done
Inline Actions

The logic stays the same, it just makes better ground for future diffs, but you're right this could have been possibly done in one of the next diffs. This is reverted anyway in D7947

When possible, it's always best to separate changes from refactor diffs... that way it's easier for the reviewer to "blindly accept" the refactor, and then pay closer attention to the actual changes

Ok, makes sense

bartek: The logic stays the same, it just makes better ground for future diffs, but you're right this…
}, [pushModal, mediaSource]);
render(): React.Node {
let progressIndicator, errorIndicator, removeButton; let progressIndicator, errorIndicator, removeButton;
const { const { multimediaImageCSSClass, multimediaCSSClass } = props;
pendingUpload,
remove,
mediaSource,
multimediaImageCSSClass,
multimediaCSSClass,
} = this.props;
if (pendingUpload) { if (pendingUpload) {
const { progressPercent, failed } = pendingUpload; const { progressPercent, failed } = pendingUpload;
if (progressPercent !== 0 && progressPercent !== 1) { if (progressPercent !== 0 && progressPercent !== 1) {
const outOfHundred = Math.floor(progressPercent * 100); const outOfHundred = Math.floor(progressPercent * 100);
const text = `${outOfHundred}%`; const text = `${outOfHundred}%`;
progressIndicator = ( progressIndicator = (
<CircularProgressbar <CircularProgressbar
value={outOfHundred} value={outOfHundred}
text={text} text={text}
background background
backgroundPadding={6} backgroundPadding={6}
className={css.progressIndicator} className={css.progressIndicator}
/> />
); );
} }
if (failed) { if (failed) {
errorIndicator = ( errorIndicator = (
<AlertCircleIcon className={css.uploadError} size={36} /> <AlertCircleIcon className={css.uploadError} size={36} />
); );
} }
if (remove) { if (removeProp) {
removeButton = ( removeButton = (
<Button onClick={this.remove}> <Button onClick={handleRemove}>
<XCircleIcon className={css.removeUpload} /> <XCircleIcon className={css.removeUpload} />
</Button> </Button>
); );
} }
} }
const imageContainerClasses = [ const imageContainerClasses = [css.multimediaImage, multimediaImageCSSClass];
css.multimediaImage,
multimediaImageCSSClass,
];
imageContainerClasses.push(css.clickable); imageContainerClasses.push(css.clickable);
// Media element is the actual image or video element (or encrypted version) // Media element is the actual image or video element (or encrypted version)
let mediaElement; let mediaElement;
if (mediaSource.type === 'photo') { if (mediaSource.type === 'photo') {
const uri = fetchableMediaURI(mediaSource.uri); const uri = fetchableMediaURI(mediaSource.uri);
mediaElement = <img src={uri} />; mediaElement = <img src={uri} />;
} else if (mediaSource.type === 'video') { } else if (mediaSource.type === 'video') {
const uri = fetchableMediaURI(mediaSource.uri); const uri = fetchableMediaURI(mediaSource.uri);
mediaElement = ( mediaElement = (
<video controls> <video controls>
<source src={uri} /> <source src={uri} />
</video> </video>
); );
} else if ( } else if (
mediaSource.type === 'encrypted_photo' || mediaSource.type === 'encrypted_photo' ||
mediaSource.type === 'encrypted_video' mediaSource.type === 'encrypted_video'
) { ) {
const { ...encryptedMediaProps } = mediaSource; const { type, holder, encryptionKey } = mediaSource;
mediaElement = <EncryptedMultimedia {...encryptedMediaProps} />; mediaElement = (
<EncryptedMultimedia
ashoatUnsubmitted
Not Done
Inline Actions

Wondering why you replaced the spread with explicit props. Is it for readability? Or does a future diff add a prop or something?

ashoat: Wondering why you replaced the spread with explicit props. Is it for readability? Or does a…
ashoatUnsubmitted
Not Done
Inline Actions

This becomes more clear in D7902. It would have been easier to review if this diff left the spread as-is, since we only needed to remove the spread in D7902 where we start passing different props

ashoat: This becomes more clear in D7902. It would have been easier to review if this diff left the…
type={type}
holder={holder}
encryptionKey={encryptionKey}
/>
);
} }
// Media node is the container for the media element (button if photo) // Media node is the container for the media element (button if photo)
let mediaNode; let mediaNode;
if ( if (mediaSource.type === 'photo' || mediaSource.type === 'encrypted_photo') {
mediaSource.type === 'photo' ||
mediaSource.type === 'encrypted_photo'
) {
mediaNode = ( mediaNode = (
<Button <Button
className={classNames(imageContainerClasses)} className={classNames(imageContainerClasses)}
onClick={this.onClick} onClick={handleClick}
> >
{mediaElement} {mediaElement}
{removeButton} {removeButton}
</Button> </Button>
); );
} else { } else {
mediaNode = ( mediaNode = (
<div className={classNames(imageContainerClasses)}>{mediaElement}</div> <div className={classNames(imageContainerClasses)}>{mediaElement}</div>
); );
} }
const containerClasses = [css.multimedia, multimediaCSSClass]; const containerClasses = [css.multimedia, multimediaCSSClass];
return ( return (
<span className={classNames(containerClasses)}> <span className={classNames(containerClasses)}>
{mediaNode} {mediaNode}
{progressIndicator} {progressIndicator}
{errorIndicator} {errorIndicator}
</span> </span>
); );
} }
remove: (event: SyntheticEvent<HTMLElement>) => void = event => { export default Multimedia;
ashoatUnsubmitted
Not Done
Inline Actions

This might be a good component to wrap with a React.memo... I assume the performance costs of rerendering media are non-negligible

ashoat: This might be a good component to wrap with a `React.memo`... I assume the performance costs of…
event.stopPropagation();
const { remove, pendingUpload } = this.props;
invariant(
remove && pendingUpload,
'Multimedia cannot be removed as either remove or pendingUpload ' +
'are unspecified',
);
remove(pendingUpload.localID);
};
onClick: () => void = () => {
const { pushModal, mediaSource } = this.props;
pushModal(<MultimediaModal media={mediaSource} />);
};
}
function ConnectedMultimediaContainer(props: BaseProps): React.Node {
const modalContext = useModalContext();
return <Multimedia {...props} pushModal={modalContext.pushModal} />;
}
export default ConnectedMultimediaContainer;