Conversation
kreeger
left a comment
kreeger
left a comment
There was a problem hiding this comment.
Sorry to get to this pull request nearly three years later! You may or may not even still care about this, but I wanted to give this a review anyway.
I appreciate this change, but before it makes it in, I'd like to see a couple things changed on it: see the changes I've requested at line-items below.
Additionally, I feel like a better model for adding a feature like this would be for the index view to ask its delegate for a configurable view (one that conforms to a protocol), and then for the index view to display that view anchored to the touch point in a particular way (maybe configurable with an enum: top/middle/bottom). I don't expect you to make any of those changes to this pull request because it's asking quite a bit! But one thing I plan on doing in the near future is rewriting this in Swift because it's woefully out of date, and I would like to add functionality for a peek view like you've done here. If you're still interested in making the changes below, that's fine, but know that they may be rewritten soon with something a bit more customizable.
| - (void)collectionIndexView:(BDKCollectionIndexView *)collectionIndexView isPressedOnIndex:(NSUInteger)pressedIndex indexTitle:(NSString *)indexTitle; | ||
| - (void)collectionIndexView:(BDKCollectionIndexView *)collectionIndexView liftedFingerFromIndex:(NSUInteger)pressedIndex; | ||
|
|
||
| - (NSAttributedString *)collectionIndexView:(BDKCollectionIndexView *)collectionIndexView index:(NSUInteger )index; |
There was a problem hiding this comment.
The name for this delegate function doesn't tell me anything about what it does. The other two indicate actions a user takes against the index view. What does this one communicate? Can you rename it to something more descriptive, please?
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| <plist version="1.0"> | ||
| <dict> | ||
| <key>IDEDidComputeMac32BitWarning</key> | ||
| <true/> | ||
| </dict> | ||
| </plist> |
There was a problem hiding this comment.
Can you remove this file from your changes?
| UIView * peekView = [self viewWithTag:9999]; | ||
| UILabel * titleLabel = [peekView viewWithTag:9998]; |
There was a problem hiding this comment.
Apple has advocated against using -viewWithTag: due to its inability to handle tag index collisions, especially when using third-party code, and this could potentially introduce collisions with other third-party code when added to somebody else's project.
Can you update this to use something other than viewWithTag? A retained reference perhaps?
| CGFloat maxOffset = self.frame.size.height - 50; | ||
| CGFloat minOffset = 0; | ||
| CGFloat peekOffset = point.y; | ||
| if (peekOffset > maxOffset){ | ||
| peekOffset = maxOffset; | ||
| } | ||
| else if (peekOffset < minOffset) { | ||
| peekOffset = minOffset; | ||
| } | ||
|
|
||
| peekView.frame = CGRectMake( (self.frame.size.width + 25) * -1, peekOffset, 50, 50); |
There was a problem hiding this comment.
No change needed here, but I just want to call this out: I would prefer the offset behavior and size of this peek view be a bit more customizable, but I think this is good for now. I may update this code to add customization options in the future.
2 participants
i adding peekView;