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.

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

Code: Improved codebase by minimizing package interdependency #12344

Merged
merged 9 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 1 addition & 1 deletion packages/animation/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
},
"dependencies": {
"@googleforcreators/i18n": "*",
"@googleforcreators/media": "*",
"@googleforcreators/elements": "*",
"@googleforcreators/react": "*",
"@googleforcreators/units": "*",
"flagged": "^2.0.6",
Expand Down
8 changes: 6 additions & 2 deletions packages/animation/src/effects/backgroundPan/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import { getElementOffsets } from '@googleforcreators/elements';

/**
* Internal dependencies
*/
import { BACKGROUND_ANIMATION_EFFECTS, DIRECTION } from '../../constants';
import SimpleAnimation from '../../parts/simpleAnimation';
import { getMediaBoundOffsets } from '../../utils';

export function EffectBackgroundPan({
panDir = DIRECTION.RIGHT_TO_LEFT,
Expand All @@ -40,7 +44,7 @@ export function EffectBackgroundPan({
const translateToOriginX = 'translate3d(0%, 0, 0)';
const translateToOriginY = 'translate3d(0, 0%, 0)';
const offsets = element
? getMediaBoundOffsets({ element })
? getElementOffsets(element)
: {
top: 0,
right: 0,
Expand Down
15 changes: 11 additions & 4 deletions packages/animation/src/effects/backgroundPanAndZoom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import {
getElementOffsets,
getElementOrigin,
} from '@googleforcreators/elements';

/**
* Internal dependencies
*/
Expand All @@ -25,7 +33,6 @@ import {
import SimpleAnimation from '../../parts/simpleAnimation';
import { EffectBackgroundPan } from '../backgroundPan';
import { EffectBackgroundZoom } from '../backgroundZoom';
import { getMediaOrigin, getMediaBoundOffsets } from '../../utils';

const defaults = {
fill: 'forwards',
Expand All @@ -50,10 +57,10 @@ export function EffectBackgroundPanAndZoom({
const animationName = `direction-${panDir}-${zoomDirection}-${BACKGROUND_ANIMATION_EFFECTS.PAN_AND_ZOOM.value}`;

// We have to move the origin with respect to the pan
// direction and the current media position relative to
// direction and the current element position relative to
// the frame. This prevents area from ever being shown
// where the media does't fill the frame during scaling
const origin = getMediaOrigin(element && getMediaBoundOffsets({ element }));
// where the element does't fill the frame during scaling
const origin = getElementOrigin(element && getElementOffsets(element));
const transformOrigin =
{
[DIRECTION.RIGHT_TO_LEFT]: {
Expand Down
9 changes: 6 additions & 3 deletions packages/animation/src/effects/backgroundZoom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
* External dependencies
*/
import { clamp, progress, lerp } from '@googleforcreators/units';
import {
getElementOffsets,
getElementOrigin,
} from '@googleforcreators/elements';

/**
* Internal dependencies
*/
import { BG_MIN_SCALE, BG_MAX_SCALE, SCALE_DIRECTION } from '../../constants';
import { AnimationZoom } from '../../parts/zoom';
import { getMediaOrigin, getMediaBoundOffsets } from '../../utils';

export function EffectBackgroundZoom({
element,
Expand Down Expand Up @@ -57,9 +60,9 @@ export function EffectBackgroundZoom({
delay,
easing,
targetLeafElement: true,
// Account for moving bg media relative to frame
// Account for moving bg element relative to frame
transformOrigin:
transformOrigin ||
getMediaOrigin(element && getMediaBoundOffsets({ element })),
getElementOrigin(element && getElementOffsets(element)),
});
}
2 changes: 0 additions & 2 deletions packages/animation/src/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,4 @@ export { defaultUnit } from './defaultUnit';
export { getGlobalSpace, literal } from './getGlobalSpace';
export { default as getOffPageOffset } from './getOffPageOffset';
export { getTotalDuration } from './getTotalDuration';
export { getMediaBoundOffsets, hasOffsets } from './mediaPositions';
export { getExclusion, orderByKeys } from './objectOperations';
export { getMediaOrigin } from './getMediaOrigin';
1 change: 0 additions & 1 deletion packages/element-library/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
"access": "public"
},
"dependencies": {
"@googleforcreators/animation": "*",
"@googleforcreators/design-system": "*",
"@googleforcreators/dom": "*",
"@googleforcreators/elements": "*",
Expand Down
14 changes: 8 additions & 6 deletions packages/element-library/src/media/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
getMediaSizePositionProps,
calculateSrcSet,
} from '@googleforcreators/media';
import { BG_MIN_SCALE, BG_MAX_SCALE } from '@googleforcreators/animation';
import {
DisplayWithMask as WithMask,
shouldDisplayBorder,
Expand Down Expand Up @@ -106,6 +105,8 @@ function MediaEdit({
getProxiedUrl,
updateElementById,
zIndexCanvas,
min,
max,
}) {
const {
id,
Expand Down Expand Up @@ -207,15 +208,12 @@ function MediaEdit({
const handleWheel = useCallback(
(evt) => {
updateLocalProperties(({ scale: oldScale }) => ({
scale: Math.min(
BG_MAX_SCALE,
Math.max(BG_MIN_SCALE, oldScale + evt.deltaY)
),
scale: Math.min(min, Math.max(max, oldScale + evt.deltaY)),
}));
evt.preventDefault();
evt.stopPropagation();
},
[updateLocalProperties]
[updateLocalProperties, min, max]
);

// Cancelable wheel events require a non-passive listener, which React
Expand Down Expand Up @@ -305,6 +303,8 @@ function MediaEdit({
height={height}
scale={scale || 100}
zIndexCanvas={zIndexCanvas}
min={min}
max={max}
/>
</Element>
);
Expand All @@ -317,6 +317,8 @@ MediaEdit.propTypes = {
getProxiedUrl: PropTypes.func,
updateElementById: PropTypes.func,
zIndexCanvas: PropTypes.object,
min: PropTypes.number.isRequired,
max: PropTypes.number.isRequired,
};

export default MediaEdit;
3 changes: 0 additions & 3 deletions packages/element-library/src/media/scalePanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import PropTypes from 'prop-types';
import styled from 'styled-components';
import { _x } from '@googleforcreators/i18n';
import { Slider } from '@googleforcreators/design-system';
import { BG_MIN_SCALE, BG_MAX_SCALE } from '@googleforcreators/animation';
import { InOverlay } from '@googleforcreators/moveable';

const MIN_WIDTH = 165;
Expand Down Expand Up @@ -67,8 +66,6 @@ function ScalePanel({
*/}
<ScaleSlider
width={width}
min={BG_MIN_SCALE}
max={BG_MAX_SCALE}
majorStep={10}
minorStep={1}
value={scale}
Expand Down
5 changes: 3 additions & 2 deletions packages/elements/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import PropTypes from 'prop-types';
import { PatternPropType } from '@googleforcreators/patterns';
import { ResourcePropTypes } from '@googleforcreators/media';
import { AnimationProps } from '@googleforcreators/animation';

/**
* Internal dependencies
Expand Down Expand Up @@ -63,7 +62,9 @@ StoryPropTypes.box = PropTypes.exact({

StoryPropTypes.page = PropTypes.shape({
id: PropTypes.string.isRequired,
animations: PropTypes.arrayOf(PropTypes.shape(AnimationProps)),
// Temporary solution for animations. Better would be to move this
// prop type to the types package, but really?
animations: PropTypes.arrayOf(PropTypes.object),
Comment on lines +65 to +67
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted, this isn't great, but okay until we convert this package to TypeScript.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better would be to move this prop type to the types package

Is this referring to just the animation prop type? Or all of StoryPropTypes in general?

Technically it would make sense for all of the StoryPropTypes.* prop types to live next to their respective TS counterparts, e.g. StoryPropTypes.page in the story package, StoryElementPropTypes in elements, and so on.

But yeah I guess with the conversion to TypeScript the prop types usage will slowly fade away anyway (unless we see a lot of value in keeping it around)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just this single prop type, because the dependency between the animation and the elements package have been reversed. Yes, it would make sense to move them all to the types package, I'll look at that in #12126.

elements: PropTypes.arrayOf(PropTypes.shape(StoryPropTypes.element)),
overlay: PropTypes.oneOf(Object.values(OverlayType)),
backgroundAudio: PropTypes.shape({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,17 @@ import { getMediaSizePositionProps } from '@googleforcreators/media';

const PRECISION = 1;

export function getMediaBoundOffsets({ element }) {
export function getElementOffsets(element) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I know that this function only makes sense for media elements (that have scale and focal values), but I've made it general anyway (just returning all zeros for non-media).

// Get elements box based on a given page size with proper ratio.
const box = getBox(element, PAGE_WIDTH, PAGE_HEIGHT);
// Calculate image offsets based off given box, focal point
// Calculate media offsets based off given box, focal point
const media = getMediaSizePositionProps(
element.resource,
box.width,
box.height,
element.scale,
element.focalX,
element.focalY
element.scale ?? 1,
element.focalX ?? 50,
element.focalY ?? 50
);

return {
Expand All @@ -68,13 +68,19 @@ export function getMediaBoundOffsets({ element }) {
}

function mapValues(obj, op) {
return Object.entries(obj).reduce((accum, [key, val]) => {
accum[key] = op(val);
return accum;
}, {});
return Object.fromEntries(
Object.entries(obj).map(([key, val]) => [key, op(val)])
);
}

export function hasOffsets({ element }) {
const offsets = getMediaBoundOffsets({ element });
export const DEFAULT_HAS_OFFSETS = {
top: false,
bottom: false,
left: false,
right: false,
};

export function getHasElementOffsets(element) {
const offsets = getElementOffsets(element);
return mapValues(offsets, (offset) => Math.abs(offset) > PRECISION);
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* External dependencies
*/
import { lerp } from '@googleforcreators/units';

/**
* Given a media element, calculates where the origin is on the media
Expand All @@ -27,7 +23,7 @@ import { lerp } from '@googleforcreators/units';
* @param {Object} offsets - story media element offsets
* @return {Object} object containing horizontal and vertical transform origin percentages
*/
export function getMediaOrigin(
function getElementOrigin(
offsets = {
top: 0,
right: 0,
Expand All @@ -38,35 +34,24 @@ export function getMediaOrigin(
// If both offsets are 0, we want the origin to be in the middle.
// Otherwise we want the percentage of one offset relative to the
// other
let vertical = 0.5;
const absOffsets = Object.entries(offsets).reduce(
(accum, [key, val]) => ({
...accum,
[key]: Math.abs(val),
}),
{}
const progress = {
vertical: 50,
horizontal: 50,
};
const absOffsets = Object.fromEntries(
Object.entries(offsets).map(([key, val]) => [key, Math.abs(val)])
);
if (!(absOffsets.top < 0.01 && absOffsets.bottom < 0.01)) {
vertical = absOffsets.top / (absOffsets.top + absOffsets.bottom);
const isSignificant = (val) => val >= 0.01;
if ([absOffsets.top, absOffsets.bottom].some(isSignificant)) {
progress.vertical =
(100 * absOffsets.top) / (absOffsets.top + absOffsets.bottom);
}
let horizontal = 0.5;
if (!(absOffsets.left < 0.01 && absOffsets.right < 0.01)) {
horizontal = absOffsets.left / (absOffsets.left + absOffsets.right);
if ([absOffsets.left, absOffsets.right].some(isSignificant)) {
progress.horizontal =
(100 * absOffsets.left) / (absOffsets.left + absOffsets.right);
}

const progress = {
vertical,
horizontal,
};

return {
horizontal: lerp(isNaN(progress.horizontal) ? 0.5 : progress.horizontal, {
MIN: 0,
MAX: 100,
}),
vertical: lerp(isNaN(progress.vertical) ? 0.5 : progress.vertical, {
MIN: 0,
MAX: 100,
}),
};
return progress;
}

export default getElementOrigin;
2 changes: 2 additions & 0 deletions packages/elements/src/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,5 @@ export {
default as duplicateElement,
getOffsetCoordinates,
} from './duplicateElement';
export * from './getElementOffsets';
export { default as getElementOrigin } from './getElementOrigin';
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
/**
* Internal dependencies
*/
import { getMediaOrigin } from '..';
import getElementOrigin from '../getElementOrigin';

describe('getMediaOrigin', () => {
describe('getElementOrigin', () => {
it.each([
[
{
Expand Down Expand Up @@ -93,6 +93,6 @@ describe('getMediaOrigin', () => {
},
],
])('given %p returns %p', (offset, origin) => {
expect(getMediaOrigin(offset)).toStrictEqual(origin);
expect(getElementOrigin(offset)).toStrictEqual(origin);
});
});
1 change: 0 additions & 1 deletion packages/media/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ export { default as createFileReader } from './createFileReader';
export { default as fetchRemoteFile } from './fetchRemoteFile';
export { default as fetchRemoteBlob } from './fetchRemoteBlob';
export { default as formatMsToHMS } from './formatMsToHMS';
export { default as generateVideoStrip } from './generateVideoStrip';
export { default as getFileNameFromUrl } from './getFileNameFromUrl';
export { default as getFirstFrameOfVideo } from './getFirstFrameOfVideo';
export { default as getImageDimensions } from './getImageDimensions';
Expand Down
17 changes: 3 additions & 14 deletions packages/rich-text/src/usePasteTextContent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,7 @@ import customExport from './customExport';
* Compare this with `useHandlePastedText` that handles pasting text while being
* in text edit-mode.
*/
// @todo this really returns the inserted element and props is Partial<Element>, so is preset.
function usePasteTextContent(
insertElement: (
type: string,
props: Record<string, unknown>,
asBackground?: boolean
) => Record<string, unknown>,
preset: Record<string, unknown>
) {
function usePasteTextContent(insertElement: (content: string) => void) {
return useCallback(
(html: string) => {
const blockMap = getPastedBlocks(html);
Expand All @@ -51,13 +43,10 @@ function usePasteTextContent(
if (!validContent) {
return false;
}
insertElement('text', {
...preset,
content: validContent,
});
insertElement(validContent);
return true;
},
[insertElement, preset]
[insertElement]
);
}

Expand Down
Loading