-
Notifications
You must be signed in to change notification settings - Fork 93
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
Feature share table view controller #30
Conversation
# Conflicts: # LineSDK/LineSDK/SharingUI/PageViewController.swift
Codecov Report
@@ Coverage Diff @@
## v5.2 #30 +/- ##
==========================================
- Coverage 79.45% 71.73% -7.73%
==========================================
Files 164 174 +10
Lines 6475 7542 +1067
==========================================
+ Hits 5145 5410 +265
- Misses 1330 2132 +802
Continue to review full report at Codecov.
|
} | ||
|
||
deinit { | ||
print("Deinit: \(self)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need to use Log.print()
to identify it is print by SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only in development and we need to remove these kind of things later when we done.
if UIApplication.shared.keyWindow?.safeAreaInsets.top == 20 { | ||
return 44 | ||
} else { | ||
return 54 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might could remove the magic numbers by using edgesForExtendedLayout = []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the embedded table view in the view controller fails to get the correct search bar height value. That is the reason why I hard coded them here. But we may try again to see whether edgesForExtendedLayout
can work or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.