Skip to content

Commit

Permalink
[web] fix stretched encrypted images
Browse files Browse the repository at this point in the history
Summary:
The cause of this bug of this bug was that multimedia modal was passing the incorrect content dimensions to the full screen view modal. We were passing the `dimensions` state (which has an initial value of `null`) to the full screen modal, so when we tried to calculate the content dimensions for the full screen modal we were always returning early and not properly setting the `dyanmicContentDimensions` for the multimedia modal.

For the solution we actually don't want to pass in the `dimensions` state at all, but instead we want to pass in the initial media dimensions so that when we try and calculate the dynamic full screen dimensions of the media we can utilize the initial media dimensions to properly calculate what the dynamic dimensions of the media will be in this full screen view.

This is also what we were doing before I made this unintentional change in the refactor:
https://github.com/CommE2E/comm/blob/8ba42dce166acf4564e2baca3ad405a21d2c7fed/web/media/multimedia-modal.react.js#L164-L191

Linear task: https://linear.app/comm/issue/ENG-5685/imagemodal-on-web-stretches-images

Test Plan:
Please see the demo video below

{F1008843}

Reviewers: bartek, ashoat

Reviewed By: ashoat

Subscribers: ashoat, tomek

Differential Revision: https://phab.comm.dev/D10208
  • Loading branch information
ginsueddy committed Dec 27, 2023
1 parent f598a55 commit 5121a05
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 23 deletions.
14 changes: 8 additions & 6 deletions web/media/encrypted-multimedia.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,14 @@ function EncryptedMultimedia(props: Props): React.Node {

if (!source && !invisibleLoad) {
loadingIndicator = props.loadingIndicatorComponent ?? (
<LoadingIndicator
status="loading"
size="large"
color="white"
loadingClassName={css.loadingIndicator}
/>
<div className={css.loadingIndicatorContainer}>
<LoadingIndicator
status="loading"
size="large"
color="white"
loadingClassName={css.loadingIndicator}
/>
</div>
);
}

Expand Down
15 changes: 4 additions & 11 deletions web/media/media.css
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,6 @@ span.multimedia > svg.progressIndicator {
height: 50px;
}

span.multimedia .loadingIndicator {
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;
margin: auto auto;
width: 25px;
height: 25px;
}

:global(.CircularProgressbar-background) {
fill: #666 !important;
}
Expand All @@ -85,3 +74,7 @@ span.multimedia .loadingIndicator {
:global(.CircularProgressbar-trail) {
stroke: transparent !important;
}

.loadingIndicatorContainer {
position: absolute;
}
4 changes: 2 additions & 2 deletions web/media/multimedia-modal.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,13 @@ function MultimediaModal(props: Props): React.Node {
const multimediaModal = React.useMemo(
() => (
<FullScreenViewModal
dynamicContentDimensions={dimensions}
initialContentDimensions={media.dimensions}
setDynamicContentDimensions={setDimensions}
>
{mediaModalItem}
</FullScreenViewModal>
),
[dimensions, mediaModalItem],
[media.dimensions, mediaModalItem],
);

return multimediaModal;
Expand Down
8 changes: 4 additions & 4 deletions web/modals/full-screen-view-modal.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import css from './full-screen-view-modal.css';

type BaseProps = {
+children: React.Node,
+dynamicContentDimensions?: ?Dimensions,
+initialContentDimensions?: ?Dimensions,
+setDynamicContentDimensions?: SetState<?Dimensions>,
};

Expand Down Expand Up @@ -79,12 +79,12 @@ class FullScreenViewModal extends React.PureComponent<Props> {
};

calculateDynamicContentDimensions: () => mixed = () => {
const { dynamicContentDimensions, setDynamicContentDimensions } =
const { initialContentDimensions, setDynamicContentDimensions } =
this.props;

if (
!this.overlay ||
!dynamicContentDimensions ||
!initialContentDimensions ||
!setDynamicContentDimensions
) {
return;
Expand All @@ -95,7 +95,7 @@ class FullScreenViewModal extends React.PureComponent<Props> {
const containerAspectRatio = containerWidth / containerHeight;

const { width: contentWidth, height: contentHeight } =
dynamicContentDimensions;
initialContentDimensions;
const contentAspectRatio = contentWidth / contentHeight;

let newWidth, newHeight;
Expand Down

0 comments on commit 5121a05

Please sign in to comment.