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

Media: Better prepend uploading items to library #12123

Merged
merged 17 commits into from
Aug 29, 2022
Merged

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Aug 16, 2022

Context

Summary

Changes the way in-progress (uploading) items are displayed in the media library to ensure they're never lost when e.g. filtering the library afterwards.

Relevant Technical Choices

To-do

User-facing changes

Uploading items are still being displayed, even when changing filters in the library:

Screen.Recording.2022-08-18.at.16.43.16.mov

Testing Instructions

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

This PR can be tested by following these steps:

  1. Upload media to the media library
  2. See uploading items in the library
  3. Filter media library by type
  4. Still see uploading items in the library
  5. Verify the uploaded items remain in library after upload finishes

Reviews

Does this PR have a security-related impact?

No

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

No

Does this PR have a legal-related impact?

No

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 #9964

@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Group: Media labels Aug 16, 2022
@timarney
Copy link
Contributor

This PR seems to works as described however noticing some odd behavior --- when filtering.

Sometimes seems to double or triple (duplicated) results

https://www.youtube.com/watch?v=ysXRQlGF1a0

@swissspidy
Copy link
Collaborator Author

OK I think I have fixed it now:

Screen.Recording.2022-08-18.at.16.43.16.mov

As you can see I was able to keep the nice benefit of keeping existing items in there when filtering by type. Improves UX a little.

@lgtm-com
Copy link

lgtm-com bot commented Aug 18, 2022

This pull request introduces 1 alert when merging d1e84e2 into 328288b - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@timarney
Copy link
Contributor

Nice ya --- re-tested and working as fully working as expected now.

@swissspidy swissspidy marked this pull request as ready for review August 19, 2022 14:56
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Aug 19, 2022

Plugin builds for f570309 are ready 🛎️!

@miina
Copy link
Contributor

miina commented Aug 22, 2022

From testing:

I'm still seeing the duplicate media issue, seems to happen with images only. To repro:

  1. Drag an image to upload while not filtering
  2. Now filter to see videos only
  3. Now filter to see images only
  4. Once uploading finishes, two of the same media item appear.
media-queue.mp4

Additional note:
while the other images are still loading, it shows only that one uploading image, I wonder if we could wait until we have the images request response and only then display all the images? Maybe it's not that important though :)

@swissspidy
Copy link
Collaborator Author

Hmm weird, I'll try to reproduce again with these steps.

Thanks for testing!

while the other images are still loading, it shows only that one uploading image, I wonder if we could wait until we have the images request response and only then display all the images? Maybe it's not that important though :)

That's a good point. It's definitely confusing. I like this suggestion.

@swissspidy
Copy link
Collaborator Author

@miina I implemented your suggestion with hiding the uploading items, but I was not able to get items to show up twice. Still happening for you?

@miina
Copy link
Contributor

miina commented Aug 25, 2022

@miina I implemented your suggestion with hiding the uploading items, but I was not able to get items to show up twice. Still happening for you?

Unfortunately yes, seeing this error, too:
Screenshot 2022-08-25 at 16 47 55

Maybe it depends on the image size, perhaps if you try with throttling? The image has to be still uploading when switching the filter back to "Image" for it to happen.

@swissspidy
Copy link
Collaborator Author

Strange. I tried with large and small images as well as network and CPU throttling without luck.

Still added some safeguards now, just in case.

Don't see how this would still happen with those in place.

Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

Seems to work now without duplicates!

@swissspidy swissspidy merged commit 9837565 into main Aug 29, 2022
@swissspidy swissspidy deleted the try/9964-prepend-items branch August 29, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Media Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media: Filtering media does not work with uploading assets
4 participants