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

New gallery view #1418

Merged
merged 10 commits into from
Aug 22, 2024
Merged

New gallery view #1418

merged 10 commits into from
Aug 22, 2024

Conversation

joshuatbrown
Copy link
Contributor

Issues covered

#1177

Description

Adds the new gallery view for posts with multiple links. You'll need to enable it in Settings under "Enable new media display" since we have lots of other things still to do:

(There are even more, but linking to all of them seems unnecessary.) The point is: this is just getting us started. We have a lot of work ahead of us. I'm not 100% sure if we should keep all of this behind the feature flag or if we should release some of it once we have it ready. I suppose that's a question for Linda.

How to test

  1. Navigate to Settings
  2. Toggle "Enable new media display" to on
  3. Find a post or two with multiple links
  4. Swipe through the links to see the orange indicator move

Screenshots/Video

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-08-21.at.15.06.20.mp4

Copy link
Member

@martindsq martindsq left a comment

Choose a reason for hiding this comment

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

The code looks very polished, nice work! I'm leaving these comments here for the moment so you can start addressing them, but I'll be testing this PR now.

Nos/Views/Components/Media/LinkPreview.swift Outdated Show resolved Hide resolved
Nos/Views/Components/Media/LinkPreview.swift Outdated Show resolved Hide resolved
Nos/Views/Components/Media/GalleryView.swift Outdated Show resolved Hide resolved
@martindsq
Copy link
Member

I think we need to assess if we have to unify the background color of the image viewer we use in the card and in the proper image viewer. Check the background color of this gif change from app bg to back.

Grabacion.de.pantalla.2024-08-21.a.la.s.5.06.16.p.m.mov

@martindsq
Copy link
Member

It's hard what's being implemented here and what will be in a future PR so feel free to tell me this will be handled in the future. I posted a post with one image and another one with two images, all three with the same dimensions. I guess the Gallery will be used in the second one. You can see in the screen capture that the ratio in the Feed chosen to display each post is different, is this ok? Will we plan to use the same ratio for single images than for image gallery?

Grabacion.de.pantalla.2024-08-21.a.la.s.5.17.00.p.m.mov

@joshuatbrown joshuatbrown requested a review from martindsq August 22, 2024 20:06
@joshuatbrown
Copy link
Contributor Author

I think we need to assess if we have to unify the background color of the image viewer we use in the card and in the proper image viewer. Check the background color of this gif change from app bg to back.

It looks like the gif uses transparency, which explains what's happening with the background color. I think this is fine for now -- it's not in the scope of this ticket -- and if we want to address it in the future, we can.

I posted a post with one image and another one with two images, all three with the same dimensions. I guess the Gallery will be used in the second one. You can see in the screen capture that the ratio in the Feed chosen to display each post is different, is this ok? Will we plan to use the same ratio for single images than for image gallery?

The way I understand, we'll use a 4:3 aspect ratio for all images that are wider than they are tall, and 3:4 for the opposite. So in the feed, everything will either be 4:3 or 3:4, whether they're individual images or images in a gallery. #1171 is where I plan to address this and make sure every image appears in the correct aspect ratio.

Copy link
Member

@martindsq martindsq left a comment

Choose a reason for hiding this comment

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

On LinkPreview, line 20, can you try adding .blendMode(.multiply) after .blur(radius: 10)

It will turn this effect

Captura de pantalla 2024-08-22 a la(s) 6 26 37 p  m

into this one

Captura de pantalla 2024-08-22 a la(s) 6 26 03 p  m

I think it makes the image stand out a bit more, what do you think?

I'm ready to approve the PR, the code looks pretty nice to me. Just let me know what you think about this idea.

@joshuatbrown
Copy link
Contributor Author

@martindsq I think you're looking at the old image viewer there. It's weird because we have the feature flag and it always defaults to "off" so you have to turn it on whenever you build. I do agree that your second screen shot looks better.

@joshuatbrown joshuatbrown requested a review from martindsq August 22, 2024 21:44
@martindsq
Copy link
Member

@martindsq I think you're looking at the old image viewer there. It's weird because we have the feature flag and it always defaults to "off" so you have to turn it on whenever you build. I do agree that your second screen shot looks better.

Oh, yeah, it was off. I now see the 4/3 aspect ratio.

Copy link
Member

@martindsq martindsq left a comment

Choose a reason for hiding this comment

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

Let's ship it 🚀

@joshuatbrown joshuatbrown added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 074f0e0 Aug 22, 2024
4 checks passed
@joshuatbrown joshuatbrown deleted the new-gallery-view branch August 22, 2024 21:59
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.

2 participants