[NavigatorIOS] Adding the ability to change the title bar font. #2242

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@lloydho

lloydho commented Aug 6, 2015

Allows custom fonts for NavigatorIOS title.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 6, 2015

Collaborator

Cool! Definitely a good feature. For API I think it might be better to have a single titleStyle prop that acts like the normal style prop:

<NavigatorIOS
  style={{ fontFamily: 'Helvetica', fontSize: 10 }}
/>

That is generally the approach used for the API of other react native components.

Collaborator

ide commented Aug 6, 2015

Cool! Definitely a good feature. For API I think it might be better to have a single titleStyle prop that acts like the normal style prop:

<NavigatorIOS
  style={{ fontFamily: 'Helvetica', fontSize: 10 }}
/>

That is generally the approach used for the API of other react native components.

@jaygarcia

This comment has been minimized.

Show comment
Hide comment
@jaygarcia

jaygarcia Aug 6, 2015

Contributor

I hope this gets merged. :)

Contributor

jaygarcia commented Aug 6, 2015

I hope this gets merged. :)

@lloydho

This comment has been minimized.

Show comment
Hide comment
@lloydho

lloydho Aug 8, 2015

Updated the component to use titleStyle instead of separate props.

lloydho commented Aug 8, 2015

Updated the component to use titleStyle instead of separate props.

@lloydho

This comment has been minimized.

Show comment
Hide comment
@lloydho

lloydho Aug 8, 2015

Note that this will be a breaking change as I also combined titleTextColor into the titleStyle.

lloydho commented Aug 8, 2015

Note that this will be a breaking change as I also combined titleTextColor into the titleStyle.

@lloydho

This comment has been minimized.

Show comment
Hide comment

lloydho commented Aug 12, 2015

ping @ide

@@ -594,6 +611,11 @@ var NavigatorIOS = React.createClass({
var shouldUpdateChild = this.state.updatingAllIndicesAtOrBeyond !== null &&
this.state.updatingAllIndicesAtOrBeyond >= i;
+ var titleStyles = precomputeStyle(flattenStyle(this.props.titleStyle));
+ if (titleStyles == null) {

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

This should be a check for !titleStyles since people sometimes pass in false for styles

@ide

ide Aug 13, 2015

Collaborator

This should be a check for !titleStyles since people sometimes pass in false for styles

React/Views/RCTWrapperViewController.m
- NSForegroundColorAttributeName: _navItem.titleTextColor
- } : nil;
+
+ NSMutableDictionary *textAttributes = [[NSMutableDictionary alloc] init];

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

minor: [NSMutableDictionary dictionary]

@ide

ide Aug 13, 2015

Collaborator

minor: [NSMutableDictionary dictionary]

React/Views/RCTWrapperViewController.m
+ if (_navItem.titleFont) {
+ textAttributes[NSFontAttributeName] = _navItem.titleFont;
+ }
+ bar.titleTextAttributes = (_navItem.titleTextColor || _navItem.titleFont) ? textAttributes : nil;

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

Change the check to textAttributes.count so it's easier to add more properties in the future

@ide

ide Aug 13, 2015

Collaborator

Change the check to textAttributes.count so it's easier to add more properties in the future

React/Views/RCTNavItemManager.m
+}
+RCT_CUSTOM_VIEW_PROPERTY(titleFontWeight, NSString, RCTNavItem)
+{
+ view.titleFont = [RCTConvert UIFont:view.titleFont withWeight:json]; // defaults to normal

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

I believe if json is nil, the weight just doesn't change if view.titleFont already has a custom weight but it should get reset to the default value here (like you did for the font size).

@ide

ide Aug 13, 2015

Collaborator

I believe if json is nil, the weight just doesn't change if view.titleFont already has a custom weight but it should get reset to the default value here (like you did for the font size).

React/Views/RCTNavItemManager.m
+}
+RCT_CUSTOM_VIEW_PROPERTY(titleFontStyle, NSString, RCTNavItem)
+{
+ view.titleFont = [RCTConvert UIFont:view.titleFont withStyle:json]; // defaults to normal

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

here too

@ide

ide Aug 13, 2015

Collaborator

here too

*/
- titleTextColor: PropTypes.string,
+ titleStyle: StyleSheetPropType({
+ color: PropTypes.string,

This comment has been minimized.

@ide

ide Aug 13, 2015

Collaborator

Can you change these to TextStylePropTypes.color etc? So like:

color: TextStylePropTypes.color,
fontFamily: TextStylePropTypes.fonFamily,
@ide

ide Aug 13, 2015

Collaborator

Can you change these to TextStylePropTypes.color etc? So like:

color: TextStylePropTypes.color,
fontFamily: TextStylePropTypes.fonFamily,
@machard

This comment has been minimized.

Show comment
Hide comment
@machard

machard Aug 18, 2015

Contributor

+1

Contributor

machard commented Aug 18, 2015

+1

React/Views/RCTNavItemManager.m
+}
+RCT_CUSTOM_VIEW_PROPERTY(titleFontStyle, NSString, RCTNavItem)
+{
+ view.titleFont = [RCTConvert UIFont:view.titleFont withStyle:json ?: @(NO)];

This comment has been minimized.

@ide

ide Aug 18, 2015

Collaborator

Shouldn't the style be a string instead of @NO? Is there a reason you pass in NO instead of @"normal"?

@ide

ide Aug 18, 2015

Collaborator

Shouldn't the style be a string instead of @NO? Is there a reason you pass in NO instead of @"normal"?

This comment has been minimized.

@lloydho

lloydho Aug 21, 2015

My bad was trying to figure out if I could pass in a value using defaultView.titleFont.___ but there wasn't anything there. The RCTConvert takes in @NO just fine though. @"normal" gets converted into @NO eventually.

@lloydho

lloydho Aug 21, 2015

My bad was trying to figure out if I could pass in a value using defaultView.titleFont.___ but there wasn't anything there. The RCTConvert takes in @NO just fine though. @"normal" gets converted into @NO eventually.

React/Views/RCTNavItemManager.m
+}
+RCT_CUSTOM_VIEW_PROPERTY(titleFontWeight, NSString, RCTNavItem)
+{
+ view.titleFont = [RCTConvert UIFont:view.titleFont withWeight:json ?: @(UIFontWeightRegular)];

This comment has been minimized.

@ide

ide Aug 18, 2015

Collaborator

This should be @"normal" instead of UIFontWeightRegular right?

@ide

ide Aug 18, 2015

Collaborator

This should be @"normal" instead of UIFontWeightRegular right?

This comment has been minimized.

@lloydho

lloydho Aug 21, 2015

Same as above.

@lloydho

lloydho Aug 21, 2015

Same as above.

+ titleFontWeight={titleStyles.fontWeight}
+ titleFontStyle={titleStyles.fontStyle}
+ titleFontFamily={titleStyles.fontFamily}
+ >

This comment has been minimized.

@ide

ide Aug 18, 2015

Collaborator

Style: Closing > goes on the same line as the last prop for opening JSX tags

@ide

ide Aug 18, 2015

Collaborator

Style: Closing > goes on the same line as the last prop for opening JSX tags

This comment has been minimized.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 18, 2015

Collaborator

Could you demo these props in the UIExplorer to show them off and also test them?

Collaborator

ide commented Aug 18, 2015

Could you demo these props in the UIExplorer to show them off and also test them?

+ fontFamily: TextStylePropTypes.fontFamily,
+ fontSize: TextStylePropTypes.fontSize,
+ fontStyle: TextStylePropTypes.fontStyle,
+ fontWeight: TextStylePropTypes.fontWeight

This comment has been minimized.

@ide

ide Aug 21, 2015

Collaborator

style nit: trailing comma on the last item when each key: value pair is on its own line

@ide

ide Aug 21, 2015

Collaborator

style nit: trailing comma on the last item when each key: value pair is on its own line

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 21, 2015

Collaborator

@lloydho Thanks for the revisions. Looks great!

@nicklockwood @a2 do one of you want to give this a quick look-over before merging it?

Collaborator

ide commented Aug 21, 2015

@lloydho Thanks for the revisions. Looks great!

@nicklockwood @a2 do one of you want to give this a quick look-over before merging it?

@idibidiart

This comment has been minimized.

Show comment
Hide comment
@idibidiart

idibidiart Sep 24, 2015

We could definitely use separate font styles for the title and the rightButton and leftButton titles.

Should be specified outside of initialRoute.

We could definitely use separate font styles for the title and the rightButton and leftButton titles.

Should be specified outside of initialRoute.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 24, 2015

Collaborator

@lloydho The process for merging PRs has been recently changed and improved so it should be quicker to merge them. Could you rebase your commit?

Collaborator

ide commented Sep 24, 2015

@lloydho The process for merging PRs has been recently changed and improved so it should be quicker to merge them. Could you rebase your commit?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 25, 2015

@lloydho updated the pull request.

@lloydho updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 25, 2015

@lloydho updated the pull request.

@lloydho updated the pull request.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Nov 25, 2015

Contributor

This looks great, however we're using titleTextColor in quite a few of our apps, and I imagine other people are as well.

Rather than removing the titleTextColor prop, can you keep it, but mark it as deprecated in favor of titleStyle, and add a console.warning if it's used?

(EDIT: to be clear, titleTextColor should still work the same as before, except that it will log a warning if used - if someone specifies both titleStyle and titleColor then maybe it can log an error to emphasise the point).

Contributor

nicklockwood commented Nov 25, 2015

This looks great, however we're using titleTextColor in quite a few of our apps, and I imagine other people are as well.

Rather than removing the titleTextColor prop, can you keep it, but mark it as deprecated in favor of titleStyle, and add a console.warning if it's used?

(EDIT: to be clear, titleTextColor should still work the same as before, except that it will log a warning if used - if someone specifies both titleStyle and titleColor then maybe it can log an error to emphasise the point).

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 30, 2015

@lloydho updated the pull request.

@lloydho updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 30, 2015

@lloydho updated the pull request.

@lloydho updated the pull request.

@idibidiart

This comment has been minimized.

Show comment
Hide comment
@idibidiart

idibidiart Nov 30, 2015

+1

We could really use this little tweak.

+1

We could really use this little tweak.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 30, 2015

@lloydho updated the pull request.

@lloydho updated the pull request.

@lloydho

This comment has been minimized.

Show comment
Hide comment
@ianor

This comment has been minimized.

Show comment
Hide comment

ianor commented Dec 14, 2015

bump

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 3, 2016

Collaborator

@lloydho - can you rebase this and ping myself and @ide? we can review and merge

Collaborator

brentvatne commented Jan 3, 2016

@lloydho - can you rebase this and ping myself and @ide? we can review and merge

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 3, 2016

Contributor

Since UINavigationBar supports arbitrary views to be used as the title, I think a better approach here might be to add a titleView property that can be an arbitrary component.

That way, if you wanted styled text for the title, you could just provide a <Text/> component with its own styles.

It's a bit more complicated to implement it this way, but it's not really any harder to use for the developer, and it's a lot more flexible.There's an example of this approach in the MapView component, where I added the ability to provide custom views for the annotation pins and callouts.

Contributor

nicklockwood commented Jan 3, 2016

Since UINavigationBar supports arbitrary views to be used as the title, I think a better approach here might be to add a titleView property that can be an arbitrary component.

That way, if you wanted styled text for the title, you could just provide a <Text/> component with its own styles.

It's a bit more complicated to implement it this way, but it's not really any harder to use for the developer, and it's a lot more flexible.There's an example of this approach in the MapView component, where I added the ability to provide custom views for the annotation pins and callouts.

@idibidiart

This comment has been minimized.

Show comment
Hide comment
@idibidiart

idibidiart Jan 3, 2016

+1 @nicklockwood totally agree esp that hybrid apps that use UINavigationController with titleView can then look and work the same in RN and pure native

+1 @nicklockwood totally agree esp that hybrid apps that use UINavigationController with titleView can then look and work the same in RN and pure native

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 3, 2016

Contributor

Sorry, it's probably unfair of me to derail this at such a late stage with a wildly different solution.

I've no objections to landing this as-is and then working on a view-based solution later. The two approaches won't conflict, and we can just deprecate titleStyle at some point if it becomes a burden to maintain both solutions.

Contributor

nicklockwood commented Jan 3, 2016

Sorry, it's probably unfair of me to derail this at such a late stage with a wildly different solution.

I've no objections to landing this as-is and then working on a view-based solution later. The two approaches won't conflict, and we can just deprecate titleStyle at some point if it becomes a burden to maintain both solutions.

@aflanagan

This comment has been minimized.

Show comment
Hide comment
@aflanagan

aflanagan Jan 9, 2016

Contributor

Any chance this will land in the 0.18 release? I'm working on an app that will be going out in a week or two and I'd love to use this!

Contributor

aflanagan commented Jan 9, 2016

Any chance this will land in the 0.18 release? I'm working on an app that will be going out in a week or two and I'd love to use this!

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jan 9, 2016

Collaborator

@aflanagan We'll cut 0.19-rc around that time which you could try.

Collaborator

ide commented Jan 9, 2016

@aflanagan We'll cut 0.19-rc around that time which you could try.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 13, 2016

@lloydho updated the pull request.

@lloydho updated the pull request.

@lloydho

This comment has been minimized.

Show comment
Hide comment
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jan 13, 2016

Collaborator
Collaborator

ide commented Jan 13, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@maqen

This comment has been minimized.

Show comment
Hide comment
@maqen

maqen Jan 30, 2016

This is a great addon, when will it get merged?

maqen commented Jan 30, 2016

This is a great addon, when will it get merged?

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 30, 2016

Contributor

I'm not sure why it failed to land after @ide shipped it. I'll look into it

Contributor

nicklockwood commented Jan 30, 2016

I'm not sure why it failed to land after @ide shipped it. I'll look into it

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Feb 11, 2016

@lloydho updated the pull request.

@lloydho updated the pull request.

@bestander

This comment has been minimized.

Show comment
Hide comment
Contributor

bestander commented Feb 15, 2016

@williambout

This comment has been minimized.

Show comment
Hide comment

👍

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 4, 2016

@mrspeaker would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

ghost commented Mar 4, 2016

@mrspeaker would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Mar 4, 2016

Contributor

@mrspeaker please ignore this silly bot

Contributor

bestander commented Mar 4, 2016

@mrspeaker please ignore this silly bot

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 7, 2016

Contributor

(The bot cc's someone who might be familiar with the code, based on blame info. We should probably change this to cc the person who commented the most on the PR other than author.)

Contributor

mkonicek commented Mar 7, 2016

(The bot cc's someone who might be familiar with the code, based on blame info. We should probably change this to cc the person who commented the most on the PR other than author.)

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 13, 2016

@lloydho updated the pull request.

@lloydho updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 15, 2016

@lloydho updated the pull request.

@lloydho updated the pull request.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 15, 2016

Contributor

Merging based on @ide accepting the PR and Nick saying:

I'm not sure why it failed to land after @ide shipped it. I'll look into it

@facebook-github-bot shipit

Contributor

mkonicek commented Mar 15, 2016

Merging based on @ide accepting the PR and Nick saying:

I'm not sure why it failed to land after @ide shipped it. I'll look into it

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 15, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

Thanks for importing. If you are an FB employee go to Phabricator to review.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2016

Contributor

Failed with a merge conflict. Can you rebase please?

Contributor

mkonicek commented Mar 16, 2016

Failed with a merge conflict. Can you rebase please?

* The default text color of the navigation bar title
*/
titleTextColor: PropTypes.string,
+ /**
+ * The style of the navigation bar title
+ */
+ titleStyle: StyleSheetPropType({

This comment has been minimized.

@ericvicenti

ericvicenti Mar 22, 2016

Contributor

Why not simply

titleStyle: StyleSheetPropType(TextStylePropTypes),
@ericvicenti

ericvicenti Mar 22, 2016

Contributor

Why not simply

titleStyle: StyleSheetPropType(TextStylePropTypes),

This comment has been minimized.

@ericvicenti

ericvicenti Mar 22, 2016

Contributor

Oh! I didn't realize you implemented these manually. Very well, then!

@ericvicenti

ericvicenti Mar 22, 2016

Contributor

Oh! I didn't realize you implemented these manually. Very well, then!

@geirman

This comment has been minimized.

Show comment
Hide comment
@geirman

geirman Mar 31, 2016

Contributor

@lloydho would you please rebase this per @mkonicek's request? Pretty sure you were the "you" he was talking to 🎱 ...

Failed with a merge conflict. Can you rebase please?

Contributor

geirman commented Mar 31, 2016

@lloydho would you please rebase this per @mkonicek's request? Pretty sure you were the "you" he was talking to 🎱 ...

Failed with a merge conflict. Can you rebase please?

@javache

This comment has been minimized.

Show comment
Hide comment
@javache

javache Aug 4, 2016

Member

This pull request hasn't seen activity in a while. If you still want to merge this, please open a new pull request.

Member

javache commented Aug 4, 2016

This pull request hasn't seen activity in a while. If you still want to merge this, please open a new pull request.

@atdrago

This comment has been minimized.

Show comment
Hide comment
@atdrago

atdrago Nov 10, 2016

This never shipped because of a merge conflict...? 😰

atdrago commented Nov 10, 2016

This never shipped because of a merge conflict...? 😰

@ericvicenti

This comment has been minimized.

Show comment
Hide comment
@ericvicenti

ericvicenti Nov 10, 2016

Contributor

Feel free to take it over as a new PR! We're still happy to ship it if anyone is willing to own it

Contributor

ericvicenti commented Nov 10, 2016

Feel free to take it over as a new PR! We're still happy to ship it if anyone is willing to own it

@iineva

This comment has been minimized.

Show comment
Hide comment
@iineva

iineva Apr 8, 2017

Contributor

I want this!😢

Contributor

iineva commented Apr 8, 2017

I want this!😢

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