Add Images to TagListView #193
Conversation
| @@ -19,3 +19,6 @@ DerivedData | |||
|
|
|||
| # CocoaPods | |||
| Pods/ | |||
| /TagListViewDemo/Images.xcassets/.DS_Store | |||
There was a problem hiding this comment.
You can use find . -name .DS_Store -print0 | xargs -0 git rm --ignore-unmatch to remove all .DS_Stores from the repository.
TagListView.podspec
Outdated
| @@ -1,6 +1,6 @@ | |||
| Pod::Spec.new do |s| | |||
| s.name = "TagListView" | |||
| s.version = "1.3.1" | |||
| s.version = "1.3.2" | |||
There was a problem hiding this comment.
No need to bump version in a pr request.
TagListView/CloseButton.swift
Outdated
| path.addLine(to: CGPoint(x: iconFrame.maxX, y: iconFrame.maxY)) | ||
| path.move(to: CGPoint(x: iconFrame.maxX, y: iconFrame.minY)) | ||
| path.addLine(to: CGPoint(x: iconFrame.minX, y: iconFrame.maxY)) | ||
| // lineWidth/2 is so the edge of the stroke is not cut off |
There was a problem hiding this comment.
Can you refine the comments here?
TagListView/TagView.swift
Outdated
| return hasOvalShape ? borderWidth : (paddingY / 2) + borderWidth | ||
| } | ||
|
|
||
| private var imagePaddingX: CGFloat { |
There was a problem hiding this comment.
imagePaddingX should come first.
TagListView/TagView.swift
Outdated
| private var imagePaddingX: CGFloat { | ||
| // If both the image and TagView are rounded, left indent by Y padding so image snugly fits on left side of TagView | ||
| // If not, indent by half padding - full padding looks terrible. | ||
| return hasOvalShape ? imagePaddingY : (paddingX / 2) |
There was a problem hiding this comment.
Could you explain how to calculate these paddings?
Also, is it possible to make these padding options @IBInspectable?
There was a problem hiding this comment.
I'll update this PR w/ better comments explaining the equaitons shortly.
As for @IBInspectable, since this is a computed variable I'm not sure it makes sense to have it be @IBInspectable.
| return hasOvalShape ? imagePaddingY : (paddingX / 2) | ||
| } | ||
|
|
||
| private var imageWidth: CGFloat { |
There was a problem hiding this comment.
Here, you are assuming image in this tag view is always rounded. Is it better to leave an option for developers?
There was a problem hiding this comment.
I recognize that's an assumption I made here. Another assumption was that if you have an image and an oval pill, you want the image tucked up on the side (as in the sample image).
The only way around that I think would be to do a new boolean flag. It would be fairly easy to add a boolean to the project for isImageFlushWithBorder (or some better-named version of that), and then use that rather than my computed hasOvalShape. I didn't want to go that heavy-handed with another group's project, but if that's the desired route I can update all this.
| } | ||
|
|
||
| private var hasOvalShape: Bool { | ||
| return layer.cornerRadius == self.frame.height / 2 |
There was a problem hiding this comment.
Naming hasOvalShape is not appropriate here.
There was a problem hiding this comment.
Can you explain more? The computed variable, or the specific name of it?
| private var imagePaddingY: CGFloat { | ||
| // If both image and TagView are rounded, image should fit snugly into corner. | ||
| // If not, image should still be larger than text. | ||
| return hasOvalShape ? borderWidth : (paddingY / 2) + borderWidth |
There was a problem hiding this comment.
Is there any top/bottom/left/right padding? Maybe using an UIEdgeInsets is much better here.
There was a problem hiding this comment.
This padding is for manual layout of the entire tag, so I was following that convention. With all the manual layout that is happening, throwing in UIEdgeInsets seems like it would add more confusion here. What do you think?
TagListView/TagView.swift
Outdated
| @@ -176,6 +196,8 @@ open class TagView: UIButton { | |||
|
|
|||
| let longPress = UILongPressGestureRecognizer(target: self, action: #selector(self.longPress)) | |||
| self.addGestureRecognizer(longPress) | |||
|
|
|||
| imageView?.contentMode = .scaleAspectFit | |||
There was a problem hiding this comment.
Do we have other options for contentMode?
There was a problem hiding this comment.
There are other options, we previously used the default, .scaleToFill. This was an accidental line that snuck in when I was hacking away, will be removed in the update coming shortly.
| @@ -18,7 +18,7 @@ class ViewController: UIViewController, TagListViewDelegate { | |||
| super.viewDidLoad() | |||
|
|
|||
| tagListView.delegate = self | |||
| tagListView.addTag("TagListView") | |||
| tagListView.addTag("TagListView", image: #imageLiteral(resourceName: "missing_person")) | |||
Feedback changes from upstream PR
Merge upstream changes
Upgraded the Pod to Swift 5
This adds the ability to put an image on the left side of
TagViews. If theTagViewis oval, the image is snug with the border. If it's not perfectly oval, it is larger than the text but still has some padding.