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.

Sign up for GitHub

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

Jump to bottom

Create a flow layout for IGList #140

Closed
wants to merge 41 commits into from
Closed

Create a flow layout for IGList #140

wants to merge 41 commits into from

Conversation

zhubofei
Copy link

@zhubofei zhubofei commented Oct 30, 2016 •
edited

Create a flow layout for IGList that does not newline sections #3

  • Minimum inter-item spacing
  • Minimum line spacing
  • Constant item size with constant layout time
  • Update layout on insert/delete/move
  • Unit Test

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.8%) to 81.661% when pulling c967195 on zhubofei:flow-layout into 9e8ad96 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.6%) to 81.913% when pulling 1b5e3e3 on zhubofei:flow-layout into 9e8ad96 on Instagram:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.6%) to 81.913% when pulling 1b5e3e3 on zhubofei:flow-layout into 9e8ad96 on Instagram:master.

@zhubofei zhubofei mentioned this pull request Oct 31, 2016
Closed
@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 81.567% when pulling 1b87766 on zhubofei:flow-layout into 9e8ad96 on Instagram:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.9%) to 81.567% when pulling 1b87766 on zhubofei:flow-layout into 9e8ad96 on Instagram:master.

@jessesquires
Copy link
Contributor

@zhubofei -- just a quick note, we'll definitely need some tests here. 😄

@rnystrom -- can we adjust settings on @coveralls ? that shit is noisy AF 🗣 🙉

@rnystrom
Copy link
Contributor

rnystrom commented Nov 1, 2016

I'm going to do a little bit of a deeper review on this to make sure I understand what all is going on, but looking at the screenshot in #3 it looks pretty cool! Some things I want to make sure we support:

  • Section insets
  • Minimum inter-item spacing
  • Minimum line spacing

That way you can make a grid layout w/ spacing between all the items (thinking of Instagram's photo grid here).

@bofeizhu
Restructure
d80cd72 
Use reference index
@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@zhubofei
Copy link
Author

zhubofei commented Nov 1, 2016

@rnystrom I updated the initial comment to include a list of tasks. I'm not quite sure if I should create a separate repo for this.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@zhubofei
Copy link
Author

zhubofei commented Nov 2, 2016

@rnystrom Also, since this layout only allows one item per section, do we really need section inset?

@rnystrom rnystrom added this to the 2.0.0 milestone Nov 11, 2016
@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage increased (+3.4%) to 90.036% when pulling 472c41a on zhubofei:flow-layout into 379d68e on Instagram:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.4%) to 90.036% when pulling 472c41a on zhubofei:flow-layout into 379d68e on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

coveralls commented Nov 12, 2016 •
edited

Coverage Status

Coverage increased (+3.4%) to 90.036% when pulling 3bf75cb on zhubofei:flow-layout into 379d68e on Instagram:master.

@zhubofei
Copy link
Author

@rnystrom IMHO, at least for fixed-size cell we can get a pretty good performance w/out cache. Actually, for large datasource, it is better to compute attributes on the go. Because layoutAttributesForElementsInRect would be called a lot more than prepareLayout and cache also takes a lot of memory.

@jessesquires jessesquires modified the milestone: 2.0.0 Nov 15, 2016
@rnystrom
Copy link
Contributor

@zhubofei oops looks like this branch got out of sync, once we're rebased we can land!

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@zhubofei
Copy link
Author

@rnystrom Can I create a new PR instead? There are too many conflicts here. It's really hard for me to rebase 😂.

@rnystrom
Copy link
Contributor

@zhubofei haha sure thing! We can close this and open a new one

@zhubofei zhubofei mentioned this pull request Nov 19, 2016
Closed
5 tasks
@zhubofei
Copy link
Author

@rnystrom Just opened #225. I'm gonna close this one.

@zhubofei zhubofei closed this Nov 19, 2016
facebook-github-bot pushed a commit that referenced this pull request Nov 21, 2016
Create a grid layout for IGList
1d3e58b 
Summary:
Rebase from #140:

Create a flow layout for IGList that does not newline sections. Closes #3

- [x] Minimum inter-item spacing
- [x] Minimum line spacing
- [x] Constant item size with constant layout time
- [x] Update layout on insert/delete/move
- [x] Unit Test
Closes #225

Differential Revision: D4211469

Pulled By: rnystrom

fbshipit-source-id: f4710dbf195701098ac50f94b6b2aa8c801b2a83
@zhubofei zhubofei deleted the flow-layout branch November 23, 2016 00:52
@otymartin
Copy link

@zhubofei So setting insets using 'IGListGridCollectionViewLayout' is currently not possible?? I'm able to create a grid but i would like some spacing on the left and right.

@zhubofei
Copy link
Author

@otymartin You can set Minimum inter-item spacing and Minimum line spacing instead.

@otymartin
Copy link

otymartin commented Dec 15, 2016 •
edited

@zhubofei
2 Issues:
Im using storyboards so for whatever reason setting minimumInterimSpace && minimumLineSpacing programmatically in my section controller does nothing. It only works via storyboards (interesting).

Secondly (Under header 2) is where I'm using the IGListGridCollectViewLayout. I have minimum line & inter-item at 4 each. As you can see, once the number of items in the row is determined (I only want 3), the excess spacing is shifted to the right instead of increasing the inter-item spacing which is the expected behaviour as shown in Mixed Data GridItem Example which uses the UICollectionViewFlowLayout && apple docs

Let me know if perhaps im doing something wrong!😓

img_3248

@zhubofei
Copy link
Author

@otymartin I think we better open an issue for this. For issue 1 can you post some code sample so that I can look into it? As for issue 2, you can refer to #263. This grid layout uses left alignment by default and we have not yet implemented center alignment like UICollectionViewFlowLayout yet.

@otymartin
Copy link

otymartin commented Dec 16, 2016 •
edited

@zhubofei you want me to open an issue for issue 1?

@jessesquires
Copy link
Contributor

let's definitely move this into it's own issue thread.

let's start with #140 (comment)

@otymartin otymartin mentioned this pull request Feb 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnystrom rnystrom rnystrom requested changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@zhubofei @facebook-github-bot @coveralls @jessesquires @rnystrom @otymartin @bofeizhu