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

Present selector as modal #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Present selector as modal #6

wants to merge 4 commits into from

Conversation

chamitha
Copy link
Owner

Present the location selector view controller as modal ala the iOS Calendar app.

Copy link

@mtnbarreto mtnbarreto left a comment

Choose a reason for hiding this comment

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

Hi @chamitha. The code looks ok to me! Let's me use it a little bit to make sure it works.
Have you tested it adding the cocoapods dependency?

LocationRow/LocationRow.swift Show resolved Hide resolved
LocationRow/LocationRow.swift Show resolved Hide resolved
@chamitha
Copy link
Owner Author

chamitha commented Mar 5, 2019

Hi @chamitha. The code looks ok to me! Let's me use it a little bit to make sure it works.
Have you tested it adding the cocoapods dependency?

Yes I did.

@@ -57,28 +57,29 @@ public final class LocationRow: Row<LocationCell>, RowType, PresenterRowType {
public override func customDidSelect() {
super.customDidSelect()

guard let controller = makeController() else { return }
guard !isDisabled else { return }

Choose a reason for hiding this comment

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

Maybe we can allow the user to set up a title and use the detailTextView as the value. By doing that the user can opt by leaving empty the title and we will get the same result as now.

I would recommend to add the cell clear button as cell accessoryView.

We also have to change cell color title when row is disabled in order to indicate that.

Take a look at switchCell to see how we do it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I would recommend to add the cell clear button as cell accessoryView.

Done in f04a8b0

Copy link
Owner Author

Choose a reason for hiding this comment

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

We also have to change cell color title when row is disabled in order to indicate that.

Done in ccd09f6

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe we can allow the user to set up a title and use the detailTextView as the value. By doing that the user can opt by leaving empty the title and we will get the same result as now.

@mtnbarreto There are a couple of reasons I would prefer the leave this as is (at least for the moment), where only the textLabel is used to display the location value / placeholder.

  1. For a consistent look and feel with the Apple location field in the Calendar app.
  2. Setting the title (textLabel) text to empty / nil does not auto left align the detailTextLabel in UITableViewCell. I can achieve this by adding constraints however the look and feel of the cell can then dynamically change from a left aligned title and right aligned value to no title and a left aligned value depending on the value of row.title. I don't particularly like this dynamic change in look and feel. This too of course can be further enhanced by allowing the user to choose the layout style, say title and value or value only but I rather implement this as a future improvement.

Let me know if these reasonings are acceptable.

@chamitha
Copy link
Owner Author

chamitha commented Apr 2, 2019

@mtnbarreto
Any update on this?

1 similar comment
@chamitha
Copy link
Owner Author

chamitha commented Jul 5, 2019

@mtnbarreto
Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants