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

Unable to set the desired page dimensions #45

Closed
teameh opened this issue May 28, 2020 · 12 comments · Fixed by #47
Closed

Unable to set the desired page dimensions #45

teameh opened this issue May 28, 2020 · 12 comments · Fixed by #47
Labels
enhancement New feature or request

Comments

@teameh
Copy link
Contributor

teameh commented May 28, 2020

Hey @fermoya, another issue from me.

I'm just playing with the component a bit to see if it fits my needs. Feel free to close these issues if these uses cases are not something you would like to support.

I'm trying to create a pager that looks something like this:

image

A pager with pages with an itemAspectRatio greater than 1 so they're wider than they're tall with only a little bit of padding on the top and bottom. Also, I would like to see quite a bit of the next page.

If I try to create this by using itemAspectRatio my pager always has a lot of padding on the top or bottom. There seems to be no way to fix this with the current attributes. I'm fairly new to SwiftUI so please correct me if there is a way to accomplish this.

I've taken your sample and added 2 sliders for the itemAspectRatio and frame.height to play with but I can't make it work.

image

The closest I get is by setting the itemAspectRatio to 0 but that gives me a pager where I can barely see the next card.

image

Wouldn't it be an option to make the pageSize an attribute the user can set directly?

.pageSize(width: CGFloat?, height: CGFloat?)

That would give a lot of freedom and open the component up to a lot more use cases!

Or even on a per page basis, that would make the component even more flexible:

Pager(
    page: self.$page1,
    data: self.data1,
    id: \.self,
    pageSize: { page -> CGSize in
         // calculate size for page
    }
) {
    self.pageView($0)
}

Looking forward to hearing your thoughts!

@fermoya
Copy link
Owner

fermoya commented May 29, 2020

@teameh , this isn't how Pager is meant to work. If you give Pager a frame of 500x500 and choose an itemAspectRatio of 0.7, it will create a page that's 350x500. You cannot pass the page size because it depends on the Pager size. So your problem is:

  • Your Pager has a width/heigh ratio lesser than 1, that is, it's longer on the vertical axis.
  • You want a page with ratio greater than 1, that is, longer on the horizontal axis.
  • You don't want vertical spacing.

That's physically not possible. What you're trying to do can be achieved like this:

GeometryReader { proxy in
    Pager(page: self.$page2,
               data: self.data2,
                id: \.self) {
                    self.pageView($0)
     }
     .itemSpacing(10)
     .padding(20)
      .background(Color.gray.opacity(0.2))
      .frame(width: min(proxy.size.width, proxy.size.height),
                  height: min(proxy.size.width, proxy.size.height) / 1.3)
}

Screenshot 2020-05-29 at 10 02 03

Notice Pager doesn't have an intrinsic size. Its size is determine by frame modifier or the available space on the screen.

@fermoya fermoya closed this as completed May 29, 2020
@teameh
Copy link
Contributor Author

teameh commented May 29, 2020

Thanks for helping me understand how it should work 😊!

But that would only work if you have space for a GeometryReader to size up the whole screen right? That won't work If I put the GeometryReader in a VStack and try to add another component above and below it:

VStack {
    Text("Another component")
    GeometryReader { proxy in
        Pager(page: self.$page1,
              data: self.data1,
              id: \.self) {
                self.pageView($0)
        }
        .itemSpacing(10)
        .padding(20)
        .background(Color.gray.opacity(0.2))
        .frame(width: min(proxy.size.width, proxy.size.height),
               height: min(proxy.size.width, proxy.size.height) / 1.3)
    }
    .frame(height: 200)
    Text("Text below")
}

Will (logically) result in:

image

I'm just trying to embed the pager in between other components without a lot of padding on the top and bottom.

I could leave the GeometryReader out and use use:

VStack {
    Text("Another component")

    Pager(page: self.$page1,
          data: self.data1,
          id: \.self) {
            self.pageView($0)
    }
        .itemSpacing(10)
        .padding(20)
        .background(Color.gray.opacity(0.2))
        .frame(height: 200)

    Text("Text below")
}

But now I don't see enough of the next page:

image

I understand that this is not how you foresaw the Pager being used but allowing these kind of results would open the component up for a lot more use cases right?

@teameh
Copy link
Contributor Author

teameh commented May 29, 2020

I get that it's not how it's meant to work, but maybe something like would work? teameh@3c22c82#commitcomment-39527479

(would need a lot more work of course, but it's just an idea)

@fermoya
Copy link
Owner

fermoya commented May 29, 2020

Pager computes the page size based on:

  • The available space
  • Item aspect ratio
  • Padding
  • Alignment insets

You'll have to find the appropriate combination of these elements to satisfy your needs. You use GeometryReader as the top most View. The problem in your example is VStack isn't expanding to the horizontal edges because nothing is pushing it to need that extra space. At the end of the day, Pager doesn't have an intrinsic size as I mentioned before

@fermoya
Copy link
Owner

fermoya commented May 29, 2020

@teameh , I appreciate your enthusiasm, but have in mind this library is not meant for a specific case. You can't specify a page size because it depends on the size of the container. You can see how the page size is defined here, and figure out what changes you need to get that size:

    var pageSize: CGSize {
        guard size != .zero else { return .zero }
        guard let itemAspectRatio = self.itemAspectRatio else {
            return CGSize(width: size.width - 2 * sideInsets, height: size.height - 2 * sideInsets)
        }

        let size = CGSize(width: self.size.width - 2 * sideInsets,
                          height: self.size.height - 2 * sideInsets)
        let side = min(size.width, size.height)
        
        if itemAspectRatio > 1 {
            return CGSize(width: side, height: side / itemAspectRatio)
        } else {
            return CGSize(width: side * itemAspectRatio, height: side)
        }
    }

Plus, the scroll offset depends on the container size. It will simply not work

@fermoya
Copy link
Owner

fermoya commented May 29, 2020

I'm noticing now that the problem in your example is that you're giving the GeometryReader a frame when you should be applying this modifier to Pager:

    var body: some View {
        VStack {
            Text("Another component")
            Pager(page: self.$page1,
                  data: self.data1,
                  id: \.self) {
                    self.pageView($0)
            }
            .itemSpacing(10)
            .padding(25)
            .background(Color.gray.opacity(0.2))
            .frame(height: 200)
            Text("Text below")
        }
    }

Screenshot 2020-05-29 at 13 08 29

And please, notice that these aren't issues of the framework but questions about SwiftUI

@fermoya
Copy link
Owner

fermoya commented May 29, 2020

@teameh I've published a 1.6.0-beta.1 adding preferredItemSize. Notice that by using this you'll be invalidating padding and itemAspectRatio. Also notice that if either the width or height are greater than the container, the default to the container's.

Hope that helps you. Technically you can achieve the same by using those two modifiers but I'll give you that this could be convenient in some cases.

@teameh
Copy link
Contributor Author

teameh commented May 29, 2020

And please, notice that these aren't issues of the framework but questions about SwiftUI

Yeah, sorry that your project had to be the first SwiftUI Component I’m trying to integrate. There’s clearly some knowledge missing on my part here.

Thanks for the info, I just logged off and I’m away this weekend but will get to this asap on Wednesday! Really awesome you pushed a beta release containing that option, looking forward to trying it out!

Cheers!

@fermoya fermoya added the enhancement New feature or request label May 29, 2020
@fermoya
Copy link
Owner

fermoya commented May 29, 2020

Sure no worries, there's always a first time. I'm trying to please everybody here, to me anyone that is interested in this library deserves my attention. But there's a compromise in the number of modifiers I can add. Every new modifier I add makes the code more difficult to maintain as there are more conditions.

Anyway, I saw your point and agree it can be a bit confusing if you need to set a constant pageSize. I'll leave the issue open till I hear from you.

@fermoya fermoya reopened this May 29, 2020
@fermoya fermoya linked a pull request May 29, 2020 that will close this issue
@teameh
Copy link
Contributor Author

teameh commented Jun 3, 2020

to me anyone that is interested in this library deserves my attention

🙏 🙌 Cheers, that's really nice!

Yeah I understand you don't just want to keep adding a bunch of modifiers.

From what I can tell from playing with it a little bit this works great. I'll continue working on this tomorrow and will let you know if I run into any issues. Again thanks for the modifier, it helps a lot!

@fermoya
Copy link
Owner

fermoya commented Jun 3, 2020

I’m glad it helped ☺️

@fermoya
Copy link
Owner

fermoya commented Jun 4, 2020

I'm closing this as it's Production ready. Feel free to open a new issue if you find bug, that way we create a new thread

@fermoya fermoya closed this as completed Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants