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

Settings: tableview section separator #91

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mkchoi212
Collaborator

mkchoi212 commented Jul 11, 2018

Checklist

  • I've run bundle exec fastlane test from the root directory to see all new and existing tests pass
  • I've followed the vlc-ios code style and run bundle exec fastlane lint to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Description

Added a separator in-between tableview sections.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:settings-header branch 2 times, most recently Jul 11, 2018

SharedSources/PresentationTheme.swift Outdated
public let tableHeader: UIFont
public init(tableHeader: UIFont) {
self.tableHeader = tableHeader

This comment has been minimized.

@Mikanbu

Mikanbu Jul 11, 2018

Member

Is there a specific reason why we would want this class to be public?

This comment has been minimized.

@mkchoi212

mkchoi212 Jul 12, 2018

Collaborator

Hmmm I guess it's unnecessary

Sources/VLCSettingsTableHeaderView.swift Outdated
@objc static let identifier = String(describing: VLCSettingsTableHeaderView.self)
@objc var headerLabel: UILabel!
@objc var separator: UIView!

This comment has been minimized.

@Mikanbu

Mikanbu Jul 11, 2018

Member

What do you think about initializing them directly here?
For example:


    @objc var separator: UIView = {
        let separator = UIView()
        separator.backgroundColor = PresentationTheme.current.colors.separatorColor
        separator.translatesAutoresizingMaskIntoConstraints = false
        return separator
    }()

This will make the init clearer by only having the addSubView() calls and the constraints stuff.

This comment has been minimized.

@mkchoi212

mkchoi212 Jul 12, 2018

Collaborator

Good point. I always forget you can do this :D

Sources/VLCSettingsTableHeaderView.swift Outdated
override init(reuseIdentifier: String?) {
super.init(reuseIdentifier: reuseIdentifier)
constraints.forEach { $0.priority = UILayoutPriority(rawValue: 999) }

This comment has been minimized.

@Mikanbu

Mikanbu Jul 11, 2018

Member

I think it's better if we use .required - 1 here.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:settings-header branch Jul 12, 2018

Sources/VLCSettingsTableHeaderView.swift Outdated
import UIKit
class VLCSettingsTableHeaderView: UITableViewHeaderFooterView {
let headerHeight: CGFloat = 1.0

This comment has been minimized.

@Mikanbu

Mikanbu Jul 12, 2018

Member

Do we need to expose this outside of this class? If not we should consider "privatizing" it 😄

@mkchoi212 mkchoi212 force-pushed the mkchoi212:settings-header branch 2 times, most recently Jul 13, 2018

SharedSources/PresentationTheme.swift Outdated
let tableHeader: UIFont
init(tableHeader: UIFont) {

This comment has been minimized.

@carolanitz

carolanitz Jul 17, 2018

Member

tableHeaderFont

- (NSString *)tableView:(UITableView *)tableView titleForHeaderInSection:(NSInteger)section
{
return nil;
}

This comment has been minimized.

@carolanitz

carolanitz Jul 17, 2018

Member

This can be deleted it will fall back to viewForHeaderInSection

This comment has been minimized.

@mkchoi212

mkchoi212 Jul 18, 2018

Collaborator

Hmmm without it, the header titles come out capitalized. I think this is because InAppSettings has a default titleForHeaderInSection

Sources/VLCSettingsTableHeaderView.swift Outdated
import UIKit
class VLCSettingsTableHeaderView: UITableViewHeaderFooterView {

This comment has been minimized.

@carolanitz

carolanitz Jul 17, 2018

Member

Could you adjust this to the new class VLCSectionTableHeaderView?

@mkchoi212 mkchoi212 force-pushed the mkchoi212:settings-header branch to e936967 Jul 18, 2018

@carolanitz

This comment has been minimized.

Member

carolanitz commented Jul 24, 2018

merged with 7ea7027 and following

@carolanitz carolanitz closed this Jul 24, 2018

@mkchoi212 mkchoi212 deleted the mkchoi212:settings-header branch Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment