Skip to content
This repository has been archived by the owner on Nov 29, 2022. It is now read-only.

show info (e.g. title/URL) on navigation bar based on a NS_OPTIONS #52

Closed
wants to merge 5 commits into from
Closed

Conversation

hesyifei
Copy link
Contributor

@hesyifei hesyifei commented Jul 7, 2016

Show info (e.g. title/URL) on navigation bar based on a NS_OPTIONS(DZNWebInfoOnNavigationBar) instead of showPageTitleAndURL used previously

BTW I think the name of this NS_OPTIONS(DZNWebInfoOnNavigationBar) can be better. Can anyone help?

…ationBar) instead of using showPageTitleAndURL
@hesyifei
Copy link
Contributor Author

hesyifei commented Jul 7, 2016

You could probably add the line break here instead: @"\n%@"

@dzenbot then if user uses DZNWebInfoOnNavigationBarURL only, the URL will not be centered inside navigation bar as the text is \n[URL]

@dzenbot
Copy link
Owner

dzenbot commented Jul 7, 2016

This looks great! Thanks for doing this @eflyjason

@dzenbot dzenbot mentioned this pull request Jul 7, 2016
@WenchaoD
Copy link

WenchaoD commented Jul 7, 2016

Generally looks good @dzenbot @eflyjason , but it would be better if we can have two optimizations:

  1. There should be users already use the property showPageTitleAndURL, so it would be more friendly if we declare it 'deprecated' rather than removing it from header.
  2. One difference between NS_ENUM and NS_OPTIONS is that the later supports bitwise operation, for example:
self.infoOnNavigationBar = DZNWebInfoOnNavigationBarURL | DZNWebInfoOnNavigationBarTitle;

which means both show url and title. But it turns out this is not working in this PR.

Thanks for this @eflyjason.

@hesyifei
Copy link
Contributor Author

hesyifei commented Jul 7, 2016

One difference between NS_ENUM and NS_OPTIONS is that the later supports bitwise operation, for example:

@WenchaoD @dzenbot I found out that other var like DZNsupportedWebActions and DZNWebNavigationTools is defined as NS_OPTIONS and I remember that work properly with |

@hesyifei
Copy link
Contributor Author

hesyifei commented Jul 7, 2016

which means both show url and title. But it turns out this is not working in this PR.

@WenchaoD Fixed in 1a2ca57. WVC.infoOnNavigationBar = DZNWebInfoOnNavigationBarURL | DZNWebInfoOnNavigationBarTitle; works properly now 😄

@hesyifei
Copy link
Contributor Author

hesyifei commented Jul 7, 2016

There should be users already use the property showPageTitleAndURL, so it would be more friendly if we declare it 'deprecated' rather than removing it from header.

bfd7414. Done :)

@hesyifei
Copy link
Contributor Author

So can merge it? 😁

@@ -52,14 +62,16 @@ typedef NS_OPTIONS(NSUInteger, DZNsupportedWebActions) {
@property (nonatomic, readwrite) DZNWebNavigationTools supportedWebNavigationTools;
/** The supported actions like sharing and copy link, add to reading list, open in Safari, etc. Default is All. */
@property (nonatomic, readwrite) DZNsupportedWebActions supportedWebActions;
/** The information to be shown on navigation bar. */
Copy link
Owner

Choose a reason for hiding this comment

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

Would be good to document which is the default value.

@hesyifei
Copy link
Contributor Author

hesyifei commented Aug 5, 2016

@dzenbot

The` All definition should be defined as Title | URL.

You mean DZNWebInfoOnNavigationBarAll = DZNWebInfoOnNavigationBarTitle | DZNWebInfoOnNavigationBarURL,?

@hesyifei
Copy link
Contributor Author

hesyifei commented Aug 5, 2016

see #56

@hesyifei hesyifei closed this Aug 5, 2016
@dzenbot
Copy link
Owner

dzenbot commented Aug 5, 2016

Yes exactly.
Why did you close the PR tho?

@WenchaoD
Copy link

WenchaoD commented Aug 5, 2016

image
😂#56

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

Successfully merging this pull request may close these issues.

None yet

3 participants