Cao issue 22 #37
Cao issue 22 #37
Conversation
There was a problem hiding this comment.
Thanks @caocmai for contributing! It looks good. Please make these above changes and re-request a review.
| @@ -215,6 +224,12 @@ class CustomDropDownView<T>: UIView, UITableViewDataSource, UITableViewDelegate, | |||
| title: items[indexPath.row].title, | |||
| lines: 0) | |||
|
|
|||
| if selectedCellIndex == "\(indexPath.row)" { | |||
There was a problem hiding this comment.
Why making the selectedCellIndex a String, can you keep it as an Int and compare? @caocmai
| @@ -28,11 +28,20 @@ class CustomDropDownView<T>: UIView, UITableViewDataSource, UITableViewDelegate, | |||
| var outsideGesture: UITapGestureRecognizer? | |||
| var insideTapGesture: UITapGestureRecognizer? | |||
|
|
|||
| var selectedCellIndex = "" | |||
| var listColor: UIColor = .lightGray | |||
| var selectedColor: UIColor = .red | |||
There was a problem hiding this comment.
Probably the default colors should be a bit more usable. Can we use something like .darkGray? Or may be any color you like and is widely acceptable?
There was a problem hiding this comment.
Also the support for the user who want to opt out of having the selected row with different background color to be added.
Just add a bool in the config if they want a different background color or not and use it here.
| return view | ||
| }() | ||
|
|
||
| lazy open var image: UIImageView = { | ||
| let image = UIImageView() |
There was a problem hiding this comment.
Please update the contentMode for the image. They look stretched.
|
|
||
| if config.dropDownCollapsable { | ||
| toggleDropDown() | ||
| tableView.reloadData() |
There was a problem hiding this comment.
Do we need a whole tableview reload? Or the previous selected row and newly selected row can be reloaded if you are doing this for the color. Make sure considering the user opted for having different selected background color or not.
| @@ -42,19 +55,20 @@ public class DropDownDisplayView: UIView { | |||
| // MARK: - Functions | |||
|
|
|||
| private func setupTitle(with tag: Int) { | |||
| title.tag = tag | |||
| // title.tag = tag | |||
There was a problem hiding this comment.
Please remove the code you've commented yourself, if that won't be used. Don't remove the old commented code though. Thanks
|
|
||
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| view.backgroundColor = .white | ||
| let ccp = CustomDropDownPresenter<String>(items: items, delegate: self) | ||
| let ccp = CustomDropDownPresenter<String>(items: items, delegate: self, listColor: .cyan, selectedColor: .yellow) |
There was a problem hiding this comment.
Same as above. Please use a more reliable set of colors, so that user's can use from the example directly.
|
Hi, thanks for the thorough feedback, I think I fixed everything. Also I decided to move the selected and background color of the dropdown in the config area instead of the view. As regard to the colors, you can change it to whatever you think it's best I was just trying to show that it works, it's not important to me. Uploaded new changes and requested new review. Thanks |
|
Sorry for getting back late @caocmai |
| lazy open var stackView: UIStackView = { | ||
| let stackView = UIStackView(arrangedSubviews: [self.image, self.title]) | ||
| stackView.distribution = .fillProportionally | ||
| return stackView | ||
| }() |
There was a problem hiding this comment.
Let me know @caocmai if you'd like to work on this issue too. Thanks!
Description
I added the ability to change the background color of the dropdown list. I also added so whenever the dropdown list has an image then show that image along with the text in the selection view.
Fixes # 22
Type of change
Strictly added only UI changes.
How Has This Been Tested?
It has been tested on the iPhone 8 and iTouch 7 by the simulator was well as on my device.
Checklist:
Reviewers:
Please tag relevant people requesting a review