Add feature to change the color for each tags #237
Conversation
nator333
commented
Aug 25, 2019
nator333
commented
- Add UIColor argument for the constructors
|
How is this going? |
TagListView/TagListView.swift
Outdated
| @@ -292,12 +292,17 @@ open class TagListView: UIView { | |||
| return CGSize(width: frame.width, height: height) | |||
| } | |||
|
|
|||
| private func createNewTagView(_ title: String) -> TagView { | |||
| private func createNewTagView(_ title: String, _ backgroundColor: UIColor? = nil) -> TagView { | |||
There was a problem hiding this comment.
Can you keep the named argument backgroundColor here? I think it increases readability.
There was a problem hiding this comment.
Thanks so much for the comment!
468738a
TagListView/TagListView.swift
Outdated
| @@ -325,6 +330,12 @@ open class TagListView: UIView { | |||
| return tagView | |||
| } | |||
|
|
|||
| @discardableResult | |||
| open func addTag(_ title: String, _ backgroundColor: UIColor? = nil) -> TagView { | |||
There was a problem hiding this comment.
Same. Please keep backgroundColor.
| } | ||
|
|
||
| @discardableResult | ||
| open func addTags(titleWithColors: [String : UIColor]) -> [TagView] { |
There was a problem hiding this comment.
Can you add some documentation for this in README?
There was a problem hiding this comment.
Please give me advice if you think of anything for the improvement.
293fa14
For different colors for each tag view
TagListView/TagListView.swift
Outdated
| } else { | ||
| tagView.tagBackgroundColor = tagBackgroundColor | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd prefer to write as tagView.tagBackgroundColor = backgroundColor ?? tagBackgroundColor.
There was a problem hiding this comment.
Also if you introduced background color to each tag, it would be a little bit vague to mention tagBackgroundColor here. I'd prefer to rename it as defaultBackgroundColor.
There was a problem hiding this comment.
Will renaming break existing code? If so, I’d suggest keeping the old name.
There was a problem hiding this comment.
Will renaming break existing code? If so, I’d suggest keeping the old name.
Yup, that could happen. Both understandable.
@xhacker @Cee @dfmarulanda Thoughts?
Plus, I wanna mention about other states dependent color properties. I ignored those this time. But I thought it's fine at this point. If somebody needs it, he'll implement it.
TagListView/TagListView.swift
Outdated
| @@ -325,6 +330,12 @@ open class TagListView: UIView { | |||
| return tagView | |||
| } | |||
|
|
|||
| @discardableResult | |||
| open func addTag(_ title: String, withBgColor backgroundColor: UIColor? = nil) -> TagView { | |||
TagListView/TagListView.swift
Outdated
| @@ -292,12 +292,17 @@ open class TagListView: UIView { | |||
| return CGSize(width: frame.width, height: height) | |||
| } | |||
|
|
|||
| private func createNewTagView(_ title: String) -> TagView { | |||
| private func createNewTagView(_ title: String, withBgColor backgroundColor: UIColor? = nil) -> TagView { | |||
There was a problem hiding this comment.
Can you replace withBgColor with withBackgroundColor?
There was a problem hiding this comment.
Thanks for the comment!
Replacing the name is easy, but please clarify the reason.
There was a problem hiding this comment.
I prefer with because it makes more sense to me.
There was a problem hiding this comment.
LGTM. I would like to remove with if the parameter is not in the first place.
Example: createNewTagView(_ title: String, backgroundColor: UIColor)
