-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added 'titleNumberOfLines' prop to control the truncating of title text #672
Conversation
By analyzing the blame information on this pull request, we identified @sarovin, @r0b1n and @lynndylanhurley to be potential reviewers |
@@ -324,9 +325,11 @@ class NavBar extends React.Component { | |||
|
|||
renderTitle(childState, index:number) { | |||
const title = this.props.getTitle ? this.props.getTitle(childState) : childState.title; | |||
const numberOfLines = this.props.titleNumberOfLines ? this.props.titleNumberOfLines : -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Javascript false == 0
, so if this.props.titleNumberofLines
is defined to 0
, it will be defaulted to -1
by this line.
In case that's what you want, I suggest this defaulting instead :
const numberOfLines = this.props.titleNumberOfLines || -1;
Otherwise :
const numberOfLines = Number.isInteger(this.props.titleNumberOfLines) ? this.props.titleNumberOfLines : -1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely spotted, I checked it out and when numberOfLines
is set to 0
no text will be shown in the header. When set to -1
it will just ignore the numberOfLines
property.
Using Number.isInteger
is cleaner imo but I don't have any strong preference for either.
I have updated the code to just check for falsy value for the prop |
@fakeyou could you rebase this and add the doc to https://github.com/aksonov/react-native-router-flux/blob/master/docs/API_CONFIGURATION.md. And I think the -1 should be 0 - can you double check that works? -1 doesnt feel right |
}, | ||
]; | ||
|
||
if (Number.isInteger(titleNumberOfLines) && titleNumberOfLines >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how you can add a conditional attribute in jsx, so I've added a check for the titleNumberOfLines
prop that will return the <Animated.Text/>
component with the numberOflines
attribute.
I would love to know if there is a better way to handle this case as now there is a lot of extra code just for a single optional attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya you can simplify this quite a bit and not have to duplicate anything:
<Component
{...(something ? {foo: 1} : {})}
/>
Want to try that and then lets see how it looks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice, I didn't know that was possible.
@joenoon Setting the attribute to For now I've added a check where it will only return the component with the |
Closing it for now. @fakeyou if you will decide to solve conflict, feel free to commit to this PR. |
As mentioned in #438 this pr adds a prop
titleNumberOfLines
to control the truncating of the title text. When set to1
it will create nice looking ellipsis for titles that are to long for the navbar.