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

Feature/visible background cells #48

Merged
merged 9 commits into from
Jun 21, 2019

Conversation

stfnhdr
Copy link
Contributor

@stfnhdr stfnhdr commented May 27, 2019

Checklist

  • I've tested my changes.
  • I've read the Contribution Guidelines.
  • I've updated the documentation if necessary.
  • I've added additional Tests where/if it makes sense.

Motivation and Context

Needed this feature for a own project an saw the help wanted on this feature in the issue tab

closes #40

tried it with different values from 1 to 10 cards on every possible device size

Description

removed this part to enable top stacking cards.
if cvMinY > cardMinY + cardHeight + minimumLineSpacing + collectionView.contentInset.top {
cvMinY = 0
}

  • added bool to set if the stacking should be on top or at the bottom

  • added int for the amout of cards shown in the background.

  • defining an newRect so cards in the background won't be removed from the collectionView

let y = collectionView.bounds.minY - cellHeight * CGFloat(topStackCount)
let newRect = CGRect(x: 0, y: y < 0 ? 0 : y, width: collectionView.bounds.maxX, height: collectionView.bounds.maxY)
let items = NSArray(array: super.layoutAttributesForElements(in: newRect)!, copyItems: true)

i am very open for improvements because this is kind of a quick shot.

@JoniVR
Copy link
Owner

JoniVR commented May 27, 2019

Hi @stfnhdr
Thanks for the PR! I'll look into testing this soon!

@stfnhdr
Copy link
Contributor Author

stfnhdr commented May 27, 2019

there's a lint warning - fixing it right now.

Stefan Haider and others added 3 commits May 27, 2019 22:24
`indexesForVisibleCards` now includes stacked cards (makes more sense)
Copy link
Owner

@JoniVR JoniVR left a comment

Choose a reason for hiding this comment

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

Ok, so it works pretty well, great work here!

I did find a few things that broke so far:

  • First off was the indexesForVisibleCards variable, that was my mistake really because it was not built for more stacked cards. I fixed that in a commit to include all cards (also stacked ones), since that makes more sense anyways.

  • I think this issue was already present, just not visible because the stack was at the bottom, I'm not sure, but since we're working on multiple card stacking I think we should slowly fade out the lowest card on the stack. It currently looks pretty weird, the card simply disappears, a slow fade out would be better I think.

ezgif com-video-to-gif

  • If the firstItemTransform property is set to a higher value (say 0.25) and we have lots of stacked cards the transform will reverse, this should be pretty easy to fix I think.

ezgif com-video-to-gif (1)

fixed issue with cards getting bigger
@stfnhdr
Copy link
Contributor Author

stfnhdr commented May 27, 2019

added your requested changes, have a good night!

@JoniVR
Copy link
Owner

JoniVR commented May 27, 2019

Don't worry about the build fail for now, I think it's just the UI Tests that are flaky from time to time and there seems to be no way to manually trigger a rebuild for PR's with travis.. I need to migrate to CircleCI or something different soon anyways..

I'll look into reviewing this in the next few days, thanks again for your awesome work on this!

@stfnhdr
Copy link
Contributor Author

stfnhdr commented May 28, 2019

Ok, i tried the tests before pushing, and they all succeeded.

Thanks, this is by far the best Solution for stacking cards i found! Great Work!

I'll put more effort into this in the future - next goal is to implement the background color fade like shazam.

@JoniVR
Copy link
Owner

JoniVR commented May 28, 2019

Hmm all seems to work well but I still think the fading needs some work, I think we need to either make only the last card (the one that's going to disappear) fade out or gradually change the color of the cells to match the background color of the VerticalCardSwiperView background. Currently it takes away from the stack effect if all cells start fading once they are not focussed anymore.

@stfnhdr
Copy link
Contributor Author

stfnhdr commented May 30, 2019

changed the fade out to the last card in the stack

@stfnhdr
Copy link
Contributor Author

stfnhdr commented Jun 17, 2019

hey, please don't forget about this pull request ;)

@JoniVR
Copy link
Owner

JoniVR commented Jun 18, 2019

Thanks for reminding me, don't worry, I haven't forgotten, it's on my TODO-list but I'm pretty busy with some other stuff right now. I'll try to review & merge it somewhere in the next few days! Thanks for all the work you've done on this!

Copy link
Owner

@JoniVR JoniVR left a comment

Choose a reason for hiding this comment

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

I'm probably going to release this as a 2.0.0 version. I might have jumped the gun on 1.0.0 so I think this is best because there will be some minor api breaking changes.

Anyways, it works very well for me, thanks so much for the PR and I'm sorry it took this long! I pledged to myself that I wouldn't be that guy but life kinda got in the way for a bit.

@JoniVR JoniVR merged commit 21e17a5 into JoniVR:master Jun 21, 2019
@stfnhdr stfnhdr deleted the feature/visible_background_cells branch July 3, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to set custom amount of visible cells in background
2 participants