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

[TIMOB-23684] iOS: Expose properties to hide the navbar #8160

Merged
merged 10 commits into from Jul 27, 2016

Conversation

kopiro
Copy link
Contributor

@kopiro kopiro commented Jul 26, 2016

@build
Copy link
Contributor

build commented Jul 26, 2016

Can one of the admins verify this patch?

@@ -304,6 +304,7 @@ - (void)viewWillAppear:(BOOL)animated; // Called when the view is about to ma
- (void)viewDidAppear:(BOOL)animated; // Called when the view has been fully transitioned onto the screen. Default does nothing
{
[self updateTitleView];
[self updateHidesBars];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I think someone broke the indentation with tabs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

tl;dr:
It's a legacy part of the code, where tabs have been used.

The way to go here is to use the pattern that is already there. All new sources have spaces by default, but some old one have tabs. The only reason we don't run a script to fix it to spaces everywhere is, to not loose the ability to git-blame the source, because it sometimes can be very important to see why a certain change was made back in the days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so tab ;)

@AngelkPetkov
Copy link
Contributor

AngelkPetkov commented Jul 26, 2016

Did a quick review, looks good just some minor changes to be addressed.

Also could you please provide a javaScript demo-code for the FT.

EDIT: Please also provide the docs update. Feature freeze for 6.0.0 is approaching (Friday). If all changes are addressed prior this could make it in 6.0.0, otherwise it goes in 6.1.0.

@hansemannn hansemannn changed the title Added properties to hide navbar [TIMOB-23684] Added properties to hide navbar Jul 26, 2016
@hansemannn hansemannn changed the title [TIMOB-23684] Added properties to hide navbar [TIMOB-23684] iOS: Expose properties to hide the navbar Jul 26, 2016
@hansemannn hansemannn added this to the 6.0.0 milestone Jul 26, 2016
[self updateHidesBarsOnSwipe];
[self updateHidesBarsOnTap];
[self updateHidesBarsWhenVerticallyCompact];
[self updateHidesBarsWhenKeyboardAppears];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation again. And do we need to call these on appear as well? I would expect that is already gets set by the setters used and the default behavior for the remaining ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to call because you can pass on constructor, not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, you're right, no need to do that.

@kopiro
Copy link
Contributor Author

kopiro commented Jul 27, 2016

@hansemannn and @AngelkPetkov - fixed and added doc in YML.

type: Boolean
default: false
since: 6.0.0
osver: {ios: "8.0"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is invalid. Check my comment again, it's

    osver: {ios: {min: "8.0"}}

@hansemannn
Copy link
Collaborator

We can merge as soon the demo-code is provided in JIRA

@hansemannn
Copy link
Collaborator

Approved!

@hansemannn hansemannn merged commit 9a5a1fa into tidev:master Jul 27, 2016
@kopiro
Copy link
Contributor Author

kopiro commented Jul 28, 2016

@hansemannn There is a constant warning with:

[ERROR] Invalid type passed to function. expected: Number, was: (null) -[TiUIWindowProxy setHidesBarsOnTap:] (TiUIWindowProxy.m:943)

I commited again, could you reopen the PR?

@hansemannn
Copy link
Collaborator

hansemannn commented Jul 28, 2016

Shit, why did it work during feature-test? It needs to be

 ourNC.hidesBarsWhenVerticallyCompact = [TiUtils boolValue:value def:NO];

for all methods. Will correct it

@kopiro
Copy link
Contributor Author

kopiro commented Jul 28, 2016

I commited with ENSURE_TYPE_OR_NIL(value, NSNumber);

The warning is about the ENSURE_TYPE

@hansemannn
Copy link
Collaborator

Well, it is correct as it is. All methods expect a boolean value like natively.

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

Successfully merging this pull request may close these issues.

None yet

4 participants