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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings view #76

Closed
wants to merge 4 commits into from
Closed

Settings view #76

wants to merge 4 commits into from

Conversation

@mkchoi212
Copy link
Collaborator

@mkchoi212 mkchoi212 commented Jun 8, 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

Settings view has been updated to match the mockups on Figma (almost)

There are still couple of things that need to be fixed but thought I would open this PR to share with you guys what I have so far 馃槃

[header addSubview:headerLabel];

UILayoutGuide *margins = [header layoutMarginsGuide];
[[headerLabel.leadingAnchor constraintEqualToAnchor:margins.leadingAnchor constant:8] setActive:true];
Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

I'm aware that this is a WIP so I'm just going to leave a single comment! 馃槃
What would you think about using [NSLayoutConstraint activateConstraints:@[];?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 8, 2018

I didn't know that was a thing! Definitely makes it look cleaner :D

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 8, 2018

If you don't mind, please put more comments on stuff! I'm kinda stuck on most things anyways 馃う鈥嶁檪锔

Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

Yes, and if we trust the documentation, it's even sometimes faster! 馃槃

@@ -33,6 +13,14 @@
<string>SETTINGS_DARKTHEME</string>
<key>Type</key>
<string>PSToggleSwitchSpecifier</string>
<key>IASKSubtitle</key>
<string>Change app colours</string>
Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 8, 2018

How do I go about adding a new string that needs to be localized??

Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

You need to add constants and their plain text counter-part to the en.lproj's "Root.strings" within the Settings.bundle.

@@ -43,6 +31,8 @@
<string>SETTINGS_PASSCODE_LOCK</string>
<key>Type</key>
<string>PSToggleSwitchSpecifier</string>
<key>IASKSubtitle</key>
<string>Secure access to your content</string>
Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 8, 2018

Same thing here. Needs localization

Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

See above :)

Copy link
Member

@Mikanbu Mikanbu left a comment

Thanks for your work, you will find some small comments/thoughts in line~

cell.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
}
}
IASK_IF_PRE_IOS6(cell.textLabel.minimumFontSize = kIASKMinimumFontSize;
Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

I think we can remove this bit of code since we don't support pre-iOS 6 anymore?

Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

oh yes!

{
VLCAboutViewController *aboutVC = [[VLCAboutViewController alloc] init];
[self.navigationController pushViewController:aboutVC animated:YES];
}

- (UIView *)settingsViewController:(id<IASKViewController>)settingsViewController tableView:(UITableView *)tableView viewForHeaderForSection:(NSInteger)section
{
UIView *header = [[UIView alloc] initWithFrame:CGRectMake(20, 0, tableView.bounds.size.width, 64)];
Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

I think we should consider using a macro at the top for the default values.
For example something like:
#define SETTINGS_HEADER_HEIGHT 64

We could also use a const for this if you prefer.

Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

Yes, that's a good idea and matches what we have in other classes.

self.modalPresentationStyle = UIModalPresentationFormSheet;
self.delegate = self;
self.showDoneButton = NO;
self.showCreditsFooter = NO;

self.navigationItem.leftBarButtonItem = [[UIBarButtonItem alloc] initWithTitle:NSLocalizedString(@"About", nil) style:UIBarButtonItemStylePlain target:self action:@selector(pushAboutViewController:)];
Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

Are you sure about the About 鈽(銈氥儺銈氣槣) string the right localization identifier?

Copy link
Member

@carolanitz carolanitz Jun 15, 2018

showAbout: as selector would be better :) The controller is not pushed


- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
return 64.0;
Copy link
Member

@Mikanbu Mikanbu Jun 8, 2018

What would you think about having a dynamic height?(mostly for accessibility reasons)
Like for example what @carolanitz did in bf01cc4

@fkuehne fkuehne self-requested a review Jun 8, 2018
@@ -33,6 +13,14 @@
<string>SETTINGS_DARKTHEME</string>
<key>Type</key>
<string>PSToggleSwitchSpecifier</string>
<key>IASKSubtitle</key>
<string>Change app colours</string>
Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

You need to add constants and their plain text counter-part to the en.lproj's "Root.strings" within the Settings.bundle.

@@ -43,6 +31,8 @@
<string>SETTINGS_PASSCODE_LOCK</string>
<key>Type</key>
<string>PSToggleSwitchSpecifier</string>
<key>IASKSubtitle</key>
<string>Secure access to your content</string>
Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

See above :)

@@ -35,16 +42,22 @@ - (instancetype)initWithStyle:(UITableViewStyle)style

- (void)viewDidLoad
{
[super viewDidLoad];

Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

馃憤

{
VLCAboutViewController *aboutVC = [[VLCAboutViewController alloc] init];
[self.navigationController pushViewController:aboutVC animated:YES];
}

- (UIView *)settingsViewController:(id<IASKViewController>)settingsViewController tableView:(UITableView *)tableView viewForHeaderForSection:(NSInteger)section
{
UIView *header = [[UIView alloc] initWithFrame:CGRectMake(20, 0, tableView.bounds.size.width, 64)];
Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

Yes, that's a good idea and matches what we have in other classes.

UIView *header = [[UIView alloc] initWithFrame:CGRectMake(20, 0, tableView.bounds.size.width, 64)];
UILabel *headerLabel = [[UILabel alloc] init];

headerLabel.font = [UIFont systemFontOfSize:23 weight:UIFontWeightSemibold];
Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

Can we generalize that font configuration similar to the way we handle colors in a global sense?

cell.accessoryType = UITableViewCellAccessoryDisclosureIndicator;
}
}
IASK_IF_PRE_IOS6(cell.textLabel.minimumFontSize = kIASKMinimumFontSize;
Copy link
Contributor

@fkuehne fkuehne Jun 8, 2018

oh yes!

@mkchoi212
Copy link
Collaborator Author

@mkchoi212 mkchoi212 commented Jun 11, 2018

With 672bda1, the privacy cell is in the same section as the theme cell as shown in the mockup!

simulator screen shot - iphone x - 2018-06-11 at 10 31 32

@mkchoi212 mkchoi212 changed the title [WIP] Settings view Settings view Jun 11, 2018
[mask.trailingAnchor constraintEqualToAnchor:margins.trailingAnchor],
[mask.topAnchor constraintEqualToAnchor:margins.topAnchor],
[mask.bottomAnchor constraintEqualToAnchor:margins.bottomAnchor],
[mask.heightAnchor constraintGreaterThanOrEqualToConstant:MINIMUM_SETTINGS_ROW_HEIGHT]
Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 13, 2018

Little hack to ensure minimum height in UITableViewCellStyleSubtitle

simulator screen shot - iphone x - 2018-06-13 at 11 20 56

Copy link
Member

@carolanitz carolanitz Jun 15, 2018

no hacks, please. If you want to ensure the correct height that leaves enough room you overwrite a cells systemLayoutSizeFittingSize or something like that. There are a bunch of articles on how to achieve this. Your implementation will not leave enough room when you have large text which is what you wanted if I understand the automatic dimension correctly. This will also make the MinimumSettingsrowheight obsolete

@mkchoi212
Copy link
Collaborator Author

@mkchoi212 mkchoi212 commented Jun 13, 2018

Final view has...

  • Section header with separator (Kind of iffy about how it looks)
  • Subtitle style for applicable tableview cells
  • Cell separator turned off
  • Minimum cell height of 64px
  • New labeling on headers
  • About button on top left corner

simulator screen shot - iphone x - 2018-06-13 at 13 06 08

@Mikanbu
Copy link
Member

@Mikanbu Mikanbu commented Jun 13, 2018

Sorry, small nitpicking again 馃槃
When in dark mode, the thumbTintColor of the switches should be changed as well like:
https://gyazo.com/ea524b9883585fb97d504c5de34705be

Copy link
Member

@carolanitz carolanitz left a comment

Looks already really good. There is a bunch of code that should probably be pulled out of the settingscontroller class and possibly even the file though :)

#import "IASKPSSliderSpecifierViewCell.h"
#import "IASKSwitch.h"
#import "IASKSpecifier.h"
#import "IASKSpecifierValuesViewController.h"
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

do we really need all of these imports ?

self.modalPresentationStyle = UIModalPresentationFormSheet;
self.delegate = self;
self.showDoneButton = NO;
self.showCreditsFooter = NO;

self.navigationItem.leftBarButtonItem = [[UIBarButtonItem alloc] initWithTitle:NSLocalizedString(@"About", nil) style:UIBarButtonItemStylePlain target:self action:@selector(pushAboutViewController:)];
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

showAbout: as selector would be better :) The controller is not pushed

@@ -125,12 +138,38 @@ - (void)updateUIAndCoreSpotlightForPasscodeSetting:(BOOL)passcodeOn

#pragma mark - IASKSettings delegate

- (void)settingsViewController:(IASKAppSettingsViewController*)sender buttonTappedForSpecifier:(IASKSpecifier*)specifier
- (IBAction)pushAboutViewController:(id)sender
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

do we need the IBAction here and the sender? I mean I don't care really but It's always a sign that something is connected over Interface Builder which is not the case here

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 18, 2018

Don't know what I was thinking :p Rookie mistake!

@@ -125,12 +138,38 @@ - (void)updateUIAndCoreSpotlightForPasscodeSetting:(BOOL)passcodeOn

#pragma mark - IASKSettings delegate

- (void)settingsViewController:(IASKAppSettingsViewController*)sender buttonTappedForSpecifier:(IASKSpecifier*)specifier
- (IBAction)pushAboutViewController:(id)sender
{
VLCAboutViewController *aboutVC = [[VLCAboutViewController alloc] init];
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

this should be presented now since it's not part of the Tableview

public let settingsSeparatorColor: UIColor
public let switchBackgroundColor: UIColor
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

I think we also need that actually in the local network screen

Copy link
Member

@carolanitz carolanitz Jul 2, 2018

^ This really needs to be consistent throughout the app. Can we adjust this in the local network screen as well as part of this PR but in a separate commit?

- (CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath
{
return UITableViewAutomaticDimension;
}
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

can go away if you set the rowheight property up top

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 27, 2018

I tried what you suggested but I think this is necessary to override the heightForRowAtIndexPath already declared in its parent class IASKSettingsViewController


UIView *separator = [[UIView alloc] initWithFrame:CGRectMake(HEADER_OFFSET, 0, header.frame.size.width - (HEADER_OFFSET * 2), 2)];
separator.backgroundColor = PresentationTheme.current.colors.settingsSeparatorColor;
[header addSubview:separator];
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

This will "break" and by that I mean the separator will be too short when you rotate the view since you set a static frame

UITableViewCell *cell = nil;
if ([identifier isEqualToString:kIASKPSToggleSwitchSpecifier]) {
cell = [[UITableViewCell alloc] initWithStyle:UITableViewCellStyleSubtitle reuseIdentifier:kIASKPSToggleSwitchSpecifier];
cell.accessoryView = [[IASKSwitch alloc] initWithFrame:CGRectMake(0, 0, 79, 27)];
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

Can we write cell subclasses for this and not cram everything in this method and viewcontroller? Also magic numbers. By having subclass in an extra file we should also be able to remove some of the above imports I reckon, which will make the entire thing more readable

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 18, 2018

VLCSettingsTableViewCell.h/m has been created!


# pragma mark - IASKAppSettingsViewController override

- (UITableViewCell*)newCellForIdentifier:(NSString*)identifier {
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

space before the *

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 18, 2018

duh 馃槃

[mask.trailingAnchor constraintEqualToAnchor:margins.trailingAnchor],
[mask.topAnchor constraintEqualToAnchor:margins.topAnchor],
[mask.bottomAnchor constraintEqualToAnchor:margins.bottomAnchor],
[mask.heightAnchor constraintGreaterThanOrEqualToConstant:MINIMUM_SETTINGS_ROW_HEIGHT]
Copy link
Member

@carolanitz carolanitz Jun 15, 2018

no hacks, please. If you want to ensure the correct height that leaves enough room you overwrite a cells systemLayoutSizeFittingSize or something like that. There are a bunch of articles on how to achieve this. Your implementation will not leave enough room when you have large text which is what you wanted if I understand the automatic dimension correctly. This will also make the MinimumSettingsrowheight obsolete

@mkchoi212 mkchoi212 changed the title Settings view [WIP] Settings view Jun 18, 2018
@mkchoi212 mkchoi212 force-pushed the settings-view branch 2 times, most recently from 0145c93 to 9d3b8a0 Jun 19, 2018
@mkchoi212 mkchoi212 changed the title [WIP] Settings view Settings view Jun 27, 2018
@objc static let identifier = String(describing: VLCSettingsTableHeaderView.self)
@objc var headerLabel: UILabel!

override var frame: CGRect {
Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jun 28, 2018

This was required to silence auto layout warnings. See https://stackoverflow.com/a/35053234/4064189

Copy link
Member

@carolanitz carolanitz Jul 2, 2018

could you try giving the constraints a priority of 999. This allows them to break temporarily while the height or width is 0

self.view.backgroundColor = PresentationTheme.current.colors.settingsBackground;
self.tableView.separatorColor = PresentationTheme.current.colors.settingsSeparatorColor;
self.view.backgroundColor = PresentationTheme.current.colors.background;
self.tableView.tintColor = PresentationTheme.current.colors.orangeUI;
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

why is this needed?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 4, 2018

whoops!

@@ -0,0 +1,73 @@
/*****************************************************************************
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

can we write new files in swift?

Copy link
Member

@carolanitz carolanitz Jul 2, 2018

also prepare for reuse is missing

public let settingsSeparatorColor: UIColor
public let switchBackgroundColor: UIColor
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

^ This really needs to be consistent throughout the app. Can we adjust this in the local network screen as well as part of this PR but in a separate commit?

get { return layer.cornerRadius }
set { layer.cornerRadius = newValue }
}
}
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

^

@objc static let identifier = String(describing: VLCSettingsTableHeaderView.self)
@objc var headerLabel: UILabel!

override var frame: CGRect {
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

could you try giving the constraints a priority of 999. This allows them to break temporarily while the height or width is 0

NSLayoutConstraint.activate([
separator.leadingAnchor.constraint(equalTo: margins.leadingAnchor, constant: offset),
separator.trailingAnchor.constraint(equalTo: margins.trailingAnchor, constant: offset),
separator.heightAnchor.constraint(equalToConstant: 2),
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

separators usually have a height of 1 or 0.5 even why the 2?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 4, 2018

Figma mockups say 2 but changing it to 1 ended up looking a lot better


headerLabel = UILabel(frame: contentView.frame)
headerLabel.translatesAutoresizingMaskIntoConstraints = false
headerLabel.font = UIFont.systemFont(ofSize: 23, weight: .semibold)
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

this font shouldn't be defined here

{
self.mainLabel.text = specifier.title;

if ([specifier.type isEqualToString:kIASKPSToggleSwitchSpecifier]) {
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

switch over the specifier.type

}
[cell configureForSpecifier:specifier value:[self.settingsStore objectForKey:specifier.key]];
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

value is too vague can we have a variable instead and a couple of checks that the object that you try to get out of there is really the type that you expect?

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 4, 2018

All type checks are done by configureForSpecifier for different types of cells with functions from InAppSettingsKit :D

if ([identifier isEqualToString:kIASKPSToggleSwitchSpecifier]) {
cell.selectionStyle = UITableViewCellSelectionStyleNone;
cell.accessoryView = [[IASKSwitch alloc] initWithFrame:CGRectZero];
[(IASKSwitch *)(cell.accessoryView) addTarget:target action:NSSelectorFromString(@"toggledValue:") forControlEvents:UIControlEventValueChanged];
Copy link
Member

@carolanitz carolanitz Jul 2, 2018

could we have the entire cell be a touch target and not the switch itself?

@mkchoi212 mkchoi212 force-pushed the settings-view branch 3 times, most recently from 0b5ace7 to c069cf8 Jul 10, 2018
@mkchoi212 mkchoi212 force-pushed the settings-view branch 2 times, most recently from c218603 to e3ada4e Jul 11, 2018
Copy link
Member

@carolanitz carolanitz left a comment

This looks already a lot better!!
I checked the PR out and constraints seem to break. I don't think you really need to have a designated xib for that since you just have the two labels in there. You can use the UITableviewCells ones for this. The subclass itself definitely makes sense. I'm not sure if Louis really wants to deviate from the standard cellheight here but if so we will find a simpler solution than creating a xib with a bunch of constraints to accomodate that and that can go into a separate PR as well

@@ -45,12 +46,15 @@ - (void)viewDidLoad
self.navigationItem.leftBarButtonItem = [[UIBarButtonItem alloc] initWithTitle:NSLocalizedString(@"BUTTON_ABOUT", nil) style:UIBarButtonItemStylePlain target:self action:@selector(showAbout)];
self.navigationItem.leftBarButtonItem.accessibilityIdentifier = VLCAccessibilityIdentifier.about;

self.neverShowPrivacySettings = YES;
self.tableView.estimatedRowHeight = 100;
Copy link
Member

@carolanitz carolanitz Jul 12, 2018

can we move the self.tableview.heightForRow = UIDimensionAutomatic here as well?

- (NSIndexPath *)tableView:(UITableView *)tableView willSelectRowAtIndexPath:(NSIndexPath *)indexPath
{
IASKSpecifier *specifier = [self.settingsReader specifierForIndexPath:indexPath];
return [specifier.type isEqual: kIASKPSSliderSpecifier] ? nil : indexPath;
Copy link
Member

@carolanitz carolanitz Jul 12, 2018

IASKSpecifier *specifier = [self.settingsReader specifierForIndexPath:indexPath];
Bool cellHasSlider = [specifier.type isEqual: kIASKPSSliderSpecifier];
return cellHasSlider ? nil : indexPath;

would that be more obvious ? We can leave it as is, just as a suggestion :)
Btw seeing the kIASKPSToggleSwitchSpecifier a few lines down as well I wonder if that shouldn't be included here

Copy link
Collaborator Author

@mkchoi212 mkchoi212 Jul 13, 2018

The kIASKPSToggleSwitchSpecifier is to allow users to toggle the switch by pressing the cell. Is this not what you meant by have the entire cell be a touch target and not the switch itself? 馃

Copy link
Member

@carolanitz carolanitz Jul 13, 2018

oh yeah you're right if we include it here the event wouldn't go through. can we just make sure that it doesn't get the grey selectionstate when you select the cell?

class VLCSettingsTableViewCell: UITableViewCell {

@IBOutlet weak var mainLabel: UILabel!
@IBOutlet weak var subtitleLabel: UILabel!
Copy link
Member

@carolanitz carolanitz Jul 12, 2018

can we not use the Textlabel and detaillabel from the UItableViewCell ?

override func prepareForReuse() {
super.prepareForReuse()
mainLabel.text = nil
subtitleLabel.text = nil
Copy link
Member

@carolanitz carolanitz Jul 12, 2018

that would happen automatically if you use the standard elements

[self themeDidChange];
}

- (void)themeDidChange
{
self.view.backgroundColor = PresentationTheme.current.colors.settingsBackground;
self.view.backgroundColor = PresentationTheme.current.colors.background;
self.tableView.separatorColor = PresentationTheme.current.colors.separatorColor;
Copy link
Member

@carolanitz carolanitz Jul 12, 2018

can be removed if the separatorStyle is none

@mkchoi212 mkchoi212 force-pushed the settings-view branch 3 times, most recently from 9302d4a to dbdd06e Jul 18, 2018
@carolanitz
Copy link
Member

@carolanitz carolanitz commented Jul 24, 2018

merged with ff9342f and following

@carolanitz carolanitz closed this Jul 24, 2018
@mkchoi212 mkchoi212 deleted the settings-view branch Jul 25, 2018
@mkchoi212
Copy link
Collaborator Author

@mkchoi212 mkchoi212 commented Jul 25, 2018

@ALL thanks for all the comments guys! this was a big one :D

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