-
Notifications
You must be signed in to change notification settings - Fork 1
Rd/pinterest #40
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
base: master
Are you sure you want to change the base?
Rd/pinterest #40
Conversation
cjc8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job getting it to work, I just want to make it a little more customizable, giving the user the ability to set the number of columns in different ways.
cjc8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it needs a rebase. A lot of the changes that have previously been merged in are not implemented.
cjc8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's something weird going on with your repo, your latest commit doesn't have anything to do with the Pinterest stuff and deletes some of the changes we've merged recently
ffef90a to
25f4a0a
Compare
cjc8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closer! Don't get discouraged, this is a big project and would take many iterations for either me or Elliot.
cjc8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closer! Have you been trying to use this layout in that test project you were using? If not, I'd recommend doing so so we can make sure it works
| offSet = columnWidth | ||
| for column in 0..<columnCount { | ||
| let width = (widthForColumn ?? { _ in return 0 })(setWidthForColumn ?? 0) ?? offSet | ||
| xOffSet.append(width * CGFloat(column)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work. The idea for the widthForColumn function is to allow users multiwidth columns, where they can be like 'I want column 1 to be 100 wide, and column 2 to be 50 wide. In that case, we we need our xOffsets to be [0, 100], but this code will generate [0,50]. Using this function will have to change the way that we generate our offsets (not just taking the width and multiplying by the column number).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Calvin,
if you haven't ran that code then do try and run it. It compiles and it works. The only thing it doesn't do is set the width for multiple sizes. I think it takes a variadic parameter though so it would be on the user to calculate and return the array.
I've ventured into making the requirement change you want and swift just doesn't support the functionality you're asking for. Swift gives me a compile error that says it cannot accept variadic tuples.
You can review the code I currently have to see what I'm doing when we meet at 12pm PST. The functionality is pretty much impossible. It's too much calculating for the compiler and trees to calculate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change your requirements to a limit like 3 columns then it should work, but you need to limit the number of columns for the iOS device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something along these lines is what I'm thinking:
func xOffsets(numCol: Int, widthForColumn: (Int) -> CGFloat?) -> [CGFloat] {
var offsets: [CGFloat] = [0.0]
for i in 0..<numCol - 1 {
let width = widthForColumn(i) ?? 50.0
offsets.append(offsets.last! + width)
}
return offsets
}
I highly doubt it has to do with whether or not it's 'possible', I think it's more likely that I'm not explaining what I want clearly enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The part we were debating about is where it broke when I texted you. The way I'm doing it makes it work. The only thing that is messed up is device orientation. However, we need to recalculate X and Y offsets on device orientation change.
I don't know what to call for that to get it to recalculate because it doesn't currently do it yet.
32ba434 to
11b9077
Compare
pinterest layout