Move to react-photo-album

[ayushnirwal]

Context

Previously react-photo-gallery was used to display media elements in the library pane, it has not seen any change since 1 July 2019. Therefore it was stuck at React version ^16.8.0 which will cause 2 versions of React to be installed if are packages published.

Summary

Documentation lacked any explanation of related to the props passed to renderPhoto ( analogous to renderImage in react-photo-gallery ). So below is an example of its props.

{
    "photo": {          // photo obj required to have key, source, width and height
        "index": 0,     // additional param added get index
        "key": 85,
        "src": "http://webstories.local/wp-content/uploads/2022/01/small-video.webm",
        "width": 560,
        "height": 320
    },
    "layoutOptions": {
        "layout": "rows",
        "viewportWidth": 1174,
        "containerWidth": 292,
        "columns": 2,
        "spacing": 4,
        "padding": 0,
        "targetRowHeight": 110
    },
    "layout": {
        "height": 81.63779527559055,  //  calculated height which can be passed down to MediaElement
        "width": 142.86614173228347,  //  calculated width which can be passed down to MediaElement
        "photoIndex": 0,              // this represents index in the current row
        "photosCount": 2            // no of photos in current row
    },
    "imageProps": {
        "src": "http://webstories.local/wp-content/uploads/2022/01/small-video.webm",
        "alt": "",
        "style": {
            "display": "block",
            "boxSizing": "content-box",
            "width": "calc((100% - 4px) / 2.01587)",
            "height": "auto",
            "aspectRatio": "560 / 320"
        },
        "className": "react-photo-album--photo",
        "sizes": "13vw"
    }
}

react-photo-album

`react-photo-gallery ( previously used )

Relevant Technical Choices

• index param was introduced to photo obj since there was no info about that in renderPhotos's props
• targetRowHeight was set to 150 to make the end result similar.
• remove mock for default export from react-photo-album in Media3pPane unit test suite.

To-do

User-facing changes

Testing Instructions

• This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

Reviews

Does this PR have a security-related impact?

Does this PR change what data or activity we track or use?

Does this PR have a legal-related impact?

Checklist

• This PR addresses an existing issue and I have linked this PR to it in ZenHub
• I have tested this code to the best of my abilities
• I have verified accessibility to the best of my abilities (docs)
• I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
• This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
• I have added documentation where necessary
• I have added a matching Type: XYZ label to the PR

---

Fixes #10444

[swissspidy]

Oh neat, thanks for picking this up!

So this is really mostly a drop-in replacement, which is great!

Curious, would the columns layout perhaps work too and cause the same results? Then we might not need to specify targetRowHeight. But I suppose we were using rows layout for a good reason... So maybe not worth testing.

Also, is there any noticeable performance improvement by any chance?

[ayushnirwal]

Also, is there any noticeable performance improvement by any chance?

Sadly no.

So this is really mostly a drop-in replacement, which is great!

Using it seems similar but it does break unit tests in packages/story-editor/src/components/library/panes/media/common/test with error TypeError: isCurrentResourceProcessing is not a function.

[swissspidy]

Using it seems similar but it does break unit tests in packages/story-editor/src/components/library/panes/media/common/test with error TypeError: isCurrentResourceProcessing is not a function.

Since you changed/removed the mock in the unit test we now have to mock isCurrentResourceProcessing in the tests as this is used by the single element being rendered.
I can take a quick look if you want.

[ayushnirwal]

I can take a quick look if you want.

Sure. Let me just push my changes to fix editor/src/components/library/panes/media/common/test/paginatedMediaGallery.js

[swissspidy]

Looks like you're on the right track. Just need to make similar change to packages/story-editor/src/components/library/panes/media/media3p/test/accesibility.js

(note: while at it, can also rename the file from accesibility.js to accessibility.js to fix the typo.

[spacedmonkey]

I am seeing very different results from original.

react-photo-gallery

react-photo-album

I don't think we should every have 3 items in one row.

[swissspidy]

3 items in a row isn't a big deal, but those large ones on a single line don't look great.

Maybe we should indeed try the columns option to see if that works better.

Let's play a little bit with the config options.

[spacedmonkey]

This is what columns looks like. I kind of like it.

[swissspidy]

Let's not decide too quickly.

This is what columns looks like. I kind of like it.

Doesn't it have accessibility considerations because items are now laid out differently? (in columns instead of rows)

Let's please test combinations of various settings first before changing from rows (which we have always used) to columns

[spacedmonkey]

I tried a number of different layouts to see what looks best. It is true, that the row layout is as similar the currently layout. But it doesn't mean we can't use column / masonry. We may want to do this in a follow up PR.

Layouts

Row layout, row height 150

masonry layout, columns 2

column layout, columns 2

[spacedmonkey]

It is also worth noting that accessibility / keyboard navigation seems to be an issue in both column and row based layouts.

I tried using keyboard navigation in both layouts, pressing arrows left and right, resulted in just going up and down in column layout and opposite in row layout.

Row layout

Screen.Recording.2022-02-07.at.11.31.02.mov

Column layout

Screen.Recording.2022-02-07.at.11.35.23.mov

[ayushnirwal]

In the rows layout, we want to avoid 3 items per row. However, I don't see an easy way to achieve that. @ayushnirwal Do you perhaps see a solution for that? We could patch react-photo-album if needed

v 1.7.0 of react-photo-album now includes a way to constraint the number of elements per row in row layout. This might help to get the desired layout.

[swissspidy]

@ayushnirwal This is amazing! At first I was like "this is a very timely update" and then I realized you made the change. Thanks a bunch! 🎉

At first glance it looks really promising. With maxPhotos: 2 I see great results. And keyboard navigation works well too.

[ayushnirwal]

In the rows layout, I often see images being stretched. Does anyone else see this? Can we avoid this?

@swissspidy do you still see this issue?
igordanchenko has left a comment which might explain it. I did not find such an issue on my end, but I have made some changes according to their suggestion.

[swissspidy]

No I don't! No stretched images or videos here.

[igordanchenko]

Apologies for jumping in here, but may I suggest you move custom renderers (including the forwardRef) to static scope outside the component? Presently renderContainer, renderRowContainer, and renderPhoto are used as functional components internally, and non-stable components refs force React to tear down and rebuild the DOM tree on every render. This is likely to result in all kinds of performance issues or flickering if PhotoAlbum is re-painted frequently.

[swissspidy]

Thanks for the suggestion, it's greatly appreciated!

[swissspidy]

This looks pretty good now!

[miina]

@swissspidy Looks like some new Karma tests are failing since this PR (at least didn't see these before when checking commits on main):
For example:

• should replace top image when bg image is set and another one is on top
• Quick Actions integration background image selected clicking the Add animation button should select the animation panel and focus the dropdown
• Quick Actions integration background image selected should add animations and filters to the background image, click the 'Reset element' button, clear the animations and filters, then click Undo and reapply the animations and filters

And some others.

Since these are related to images then seem like valid failures.

[swissspidy]

I could swear I checked Kama, not sure how I missed that 🤔

I'll look into it, but probably not today. If you have a spare moment to briefly run them locally to see whee they fail, that'd be helpful. Thanks!

[miina]

I'll look into it, but probably not today. If you have a spare moment to briefly run them locally to see whee they fail, that'd be helpful. Thanks!

At least this is failing locally as well:
should replace top image when bg image is set and another one is on top

The Quick Actions tests mentioned above passed locally (but fail on the PRs).

I've actually also mostly finished for today so wouldn't have time to check it in more detail until Monday.

[swissspidy]

Apparently what all the failing tests have in common is that they try dragging a media library item to the edge of the canvas to set it as the bg. And that doesn't work anymore.

I can confirm that drop-targets are not (reliably?) working in the editor when dragging straight from the media library.

@miina You know that code well, perhaps something comes to mind?

[miina]

I can confirm that drop-targets are not (reliably?) working in the editor when dragging straight from the media library.

Hm, I can actually see it working reliably locally 🤔 Is it sometimes working and sometimes not in your case?

[miina]

It wouldn't work when

• LibraryMoveable is not displayed for some reason (e.g. getting the active state -- hovering or focusing -- of the item is not detected)
• LibraryMoveable is displayed but something else is covering it, interfering with the events.

[swissspidy]

Weird, at first it didn't work at all, but now it seems just fine reliably. Still not working in Karma tests though.

This is the relevant bit:

web-stories-wp/packages/story-editor/src/components/dropTargets/karma/order.karma.js

Lines 47 to 56 in 928311c

	// Drag first media element straight to canvas edge to set as background
	const bgMedia = fixture.editor.library.media.item(0);
	const canvas = fixture.editor.canvas.framesLayer.fullbleed;
	await fixture.events.mouse.seq(({ down, moveRel, up }) => [
	moveRel(bgMedia, 20, 20),
	down(),
	moveRel(canvas, 10, 10),
	up(),
	]);

[miina]

This is the relevant bit:

Would be great to inspect the elements in DOM in Karma tests while the cursor is above a media item to see if the LibraryMoveable is displayed as expected. And if anything covers it now that didn't before.

Perhaps it's possible to take a snapshot at that moment and then inspect the source locally?

[miina]

Ah, actually -- do you know if it's about the drop-targets not working or about the element not being dragged at all?
I thought it was about the element not being dragged but I now realize you mentioned that for you the drop-targets were not working 🤔

[igordanchenko]

@miina,

Ah, actually -- do you know if it's about the drop-targets not working or about the element not being dragged at all?

In local tests the element is being dragged just fine.

But it looks like the issue here is that the handleDrag callback has no dropTargets as there appears to be a race condition between InnerElement and registerDropTarget. When registerDropTarget is called first, everything is working as expected. But when InnerElement is rendered before registerDropTarget is called, then dragHandler callback references stale dropTargets which is empty.

web-stories-wp/packages/story-editor/src/components/library/panes/media/common/innerElement.js

Lines 115 to 132 in 7fc8dfe

	const { handleDrag, handleDrop } = useDropTargets(
	({ actions: { handleDrag, handleDrop } }) => ({
	handleDrag,
	handleDrop,
	}),
	() => {
	// If we're dragging this element, always update the actions.
	if (hasSetResourceTracker.current) {
	return false;
	// If we're rendering the first time, init `handleDrag` and `handleDrop`.
	} else if (hasSetResourceTracker.current === null) {
	hasSetResourceTracker.current = false;
	return false;
	}
	// Otherwise ignore the changes in the actions.
	return true;
	}
	);