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

Add support for width modes other than 1/n #12

Open
diegorozen opened this issue Jan 6, 2019 · 9 comments
Open

Add support for width modes other than 1/n #12

diegorozen opened this issue Jan 6, 2019 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@diegorozen
Copy link
Contributor

Currently, all widths are represented as a fraction 1/n of the width. How would you suggest to add support for something like |-- 2/3 --|-- 1/3 --| : Redefining fractionalWidth to use a floating point number, or adding a 'numerator' associated value to that enum?

Thanks!

@diegorozen
Copy link
Contributor Author

I noticed as well that the layout only tries to fit elements in the same row as long as they have equal size. (A row composed of 1/2 + 1/4 + 1/4 would be split into 2 rows)

image

@EdwinXiang

This comment has been minimized.

@bryankeller bryankeller self-assigned this Jan 8, 2019
@bryankeller bryankeller added the enhancement New feature or request label Jan 8, 2019
@bryankeller
Copy link
Contributor

bryankeller commented Jan 10, 2019

@diegorozen Thanks for the suggestion! Here are some initial thoughts:

  • Should we support a self-sizing size mode in the horizontal direction? If so, this would naturally be a "packing layout" where items pack horizontally until they run out of space. This would essentially be flow layout's behavior.

I think the answer to this is - yes.

  • Should we support a width mode that defines item widths as any % of the width, rather than by specifying just the divisor (making a 3/4 width mode item possible)

I think the answer to this is - yes.

  • Should we support packing behavior for the existing halfWidth / thirdWidth / fourthWidth / etc width modes, which isn't currently possible since only same-width-mode items are put in the same row?

Probably. My only hesitation is API clarity, but that's hopefully a solvable problem.

  • Can all of this be implemented in an API / backwards compatible way?

I'm not sure yet. I am a bit concerned about this, as it would require all consumers of MagazineLayout to re-think how their size modes are implemented. If this could be implemented in a purely additive way, without compromising on long-term API clarity, that would be ideal.


I'm going to do some explorations on what the API could look like to get feedback. Thanks again for suggesting this!

@diegorozen
Copy link
Contributor Author

@bryankeller Agree on #1, #2. On #3, why would make a difference for any existing users? Do you rely on specifying a different width to guarantee to cell to be on a new row or this is just an artifact of the current behavior? As you said before, emulating the default flow layout behavior would be what users of the library would expect

Thanks again for making this library and sharing it!

@fruitcoder
Copy link
Contributor

  1. I agree, but Apple's flow layout deals very weirdly with rows where items don't consume all the horizontal space. It's an interpretation of minimumInteritemSpacing but one that I really hate. The solution MagazineLayout goes for should ideally allow tag layouts by default while having a way to also achieve Apple's flow layout behavior. We could do this with a SpaceDistribution/RowSpaceDistribution etc. enum that can be specified per section via the delegate. Since the sizes are all calculated before the implementation would be just adjusting the attributes' origin.x values according to the case (leadingSpace, trailingSpace (proposed default), equalSpacing (Apple), surroundingSpace (centering)).

  2. Yes

  3. Yes

  4. It's possible but it's not nice. One additional case for MagazineLayoutItemWidthMode will be dynamic so that's not an issue. The question is how we handle the additional feature. It's possible to add an additional customFrationalWidth with a CGFloat as associated value. I can't think of a better name since fractionalWidth seems ideal 😄 This would confuse the framework consumers since it's not clear what the difference is. But it also would add unnecessary complexity in the implementation since fractionalWidth would just be a specialisation of customFractionalWidth but had to handle both. So it might just be better to go for a breaking change and rename the associated value+type so we have case fractionalWidth(fraction: CGFloat). If users have only used the halfWidth, thirdWidth etc. convenience getters then they shouldn't even notice the api change.

Side note to SpaceDistribution: MagazineLayout already does this better than the flow layout for the vertical alignment of items within a row by aligning vertically smaller items to the top (haven't we all created a TopAlignedFlowLayout subclass 🙈) instead of centering them in the row (looking at you flow layout 👀). If there is a need for achieving similar flexibility as for horizontal space distribution there could be HorizontalSpaceDistribution and VerticalSpaceDistribution with trailingSpace and bottomSpace as default.

@evil159
Copy link

evil159 commented Jan 15, 2019

I'd like to suggest also adding one more width mode that follows readable content guide width. This should be handy for displaying text as well as table view-like collection views.
For that HorizontalSpaceDistribution would also be needed, as such cells would probably mostly be centred.

@fruitcoder
Copy link
Contributor

Can't you use .fullWidth and just constrain the label/textView to the readableContentGuide of the cell?

@evil159
Copy link

evil159 commented Jan 15, 2019

Yes, it is possible to do it like that programmatically. However, it is not really an option when using Interface Builder - there is Follow Readable Width checkbox which does not seem to work for collection view cell's subviews.

@evil159
Copy link

evil159 commented Feb 6, 2019

Just replying to myself - then the Interface Builder is not quite a good fit for that case. It looks more like a problem with Apple developer toolkit, rather than a problem of this library.

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

No branches or pull requests

5 participants