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

feat: fullscreen images slideshow #1120

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

aleensd
Copy link
Collaborator

@aleensd aleensd commented Nov 18, 2024

Description

Steps

Pre-deploy

Post-deploy

@aleensd aleensd linked an issue Nov 18, 2024 that may be closed by this pull request
@aleensd aleensd marked this pull request as draft November 18, 2024 16:00
@mgogoulos
Copy link
Contributor

Great to see this! Is it ready for testing in demo? Make sure you push the static files as well!!!

@aleensd
Copy link
Collaborator Author

aleensd commented Nov 18, 2024

@mgogoulos i still need to clean up the code a bit test it more and add keyboard keys functionalities

@aleensd aleensd marked this pull request as ready for review November 19, 2024 08:55
@mgogoulos
Copy link
Contributor

mgogoulos commented Nov 20, 2024

This looks great so far!

I've made these two changes and pushed them in main, can you sync in order to get them?

  1. the slideshow_items now returns the first item as the image itself. Thus when you click on the image, you get that image as the first fullscreen item. Before this, you would get another image as first, and that would be strange!
  2. I've made the number of slideshow items as configurable, checkout files/models.py on slideshow_items = getattr(settings, "SLIDESHOW_ITEMS", 30)

It expects a setting SLIDESHOW_ITEMS and will read that number, otherwise it sets a default of 30.
If you want, give it a test, eg upload a larger number of images, and increase this number to see how they look

Other than that, I have the following suggestions:

  • on the media page, when you hover on the image, show a message (eg 'load fullscreen'), the indication is good but i think this is also needed
  • show the title of media, eg below the image, since we have it on the slideshow_items dictionary
  • I noticed the following: if i visit a page that has a large image, it starts showing, before it completes if i select another one, it will wait until the first is downloaded and show it before it proceeds. Could we somehow cancel the first request, when there is a change? I noticed that requests are canceled when i click 2-3 times right (not sure if you are doing this, or if it's the browser doing it) but this is nice behavior I think and we should include it in the scenario i describe (hope it makes sense btw!). The idea is if we can make the transition a bit smoother, let's also add the title and see how it looks, because you will see there is the title that has changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fullscreen for images + support for slideshow
2 participants