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

Add demo app #30

Merged
merged 5 commits into from
Aug 15, 2019
Merged

Add demo app #30

merged 5 commits into from
Aug 15, 2019

Conversation

captainbarbosa
Copy link
Contributor

Workin on it

@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch 3 times, most recently from be1fabb to 94d58d6 Compare August 1, 2019 00:20
@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch from a479c16 to a71a4ec Compare August 7, 2019 16:31
@captainbarbosa captainbarbosa marked this pull request as ready for review August 7, 2019 17:19
@captainbarbosa
Copy link
Contributor Author

I'm going to leave the features introduced from #43 for a separate PR.

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

This looks good! I have two main architecture observations:

For delegate implementations try to follow the pattern:

extension ClassName: DelegateName {
...
}

When returning the updated view, avoid using willMove and move that logic to the controller.

I found two bugs:

In the line style annotations when you pick a different color then close then chose again the annotation it resets the previous selected value.

On non iPhone X phones the content insets at the bottom are wrong.
iPhone8

@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch 2 times, most recently from 4b2bacf to 9edcb29 Compare August 9, 2019 20:19
@captainbarbosa captainbarbosa mentioned this pull request Aug 12, 2019
13 tasks
@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch from 7f246a2 to 1341b3f Compare August 14, 2019 00:14
@captainbarbosa
Copy link
Contributor Author

In the line style annotations when you pick a different color then close then chose again the annotation it resets the previous selected value.

@fabian-guerra I pulled in #47 in 1341b3f which fixes this issue.

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

This looks good, and almost done. There are three remaining issues:

  1. If you select the label and change only the font color it changes the icon color too.
  2. If you select one annotation then select another then hold and drag the previous annotation it duplicates that annotation.
  3. Is not possible to change the icon rotation in non iPhoneX phones.

@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch from b8651c4 to 0241c01 Compare August 15, 2019 19:35
@captainbarbosa
Copy link
Contributor Author

captainbarbosa commented Aug 15, 2019

Issues 1 & 3 are fixed in 0241c01, and 2 will resolve itself once #48 merges and is pulled back into this branch.

@captainbarbosa captainbarbosa force-pushed the live-from-new-york-its-a-demo-app branch from 0241c01 to 4782b93 Compare August 15, 2019 20:31
Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

Thank you.

@captainbarbosa captainbarbosa merged commit 0588a64 into master Aug 15, 2019
@captainbarbosa captainbarbosa deleted the live-from-new-york-its-a-demo-app branch August 15, 2019 21:28
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