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

badge support #70

Merged
merged 12 commits into from Feb 11, 2018
Merged

badge support #70

merged 12 commits into from Feb 11, 2018

Conversation

@ShayanJavadi
Copy link
Contributor

@ShayanJavadi ShayanJavadi commented Feb 4, 2018

Added badge support. This also adds react-native-material-ui as a dependency.
screen shot 2018-02-04 at 2 22 45 am

#52

added badge support
@ShayanJavadi ShayanJavadi mentioned this pull request Feb 4, 2018
@timomeh
Copy link
Owner

@timomeh timomeh commented Feb 6, 2018

Thank you for your PR. While I don't have anything against dependencies, react-native-material-ui is quite a big dependency just to use their Badge.

Implementing a Badge without react-native-material-ui wouldn't be that hard, I think.

I would accept this PR without react-native-material-ui, but at the current state I can't justify shipping this with such a big dependency for one feature.

@ShayanJavadi
Copy link
Contributor Author

@ShayanJavadi ShayanJavadi commented Feb 6, 2018

That makes sense. I will update the pr by implementing a badge and removing the dependency then. Stay tuned.

ShayanJavadi added 8 commits Feb 7, 2018
removed react-native-material-ui as dependency. implemented importing local badge component.
Added Badge component to use instead of react-native-material-ui badge
updated prop types
removed old prop type
documented badge changes
fixed isBadgeVisible documentation
final changes
final changes
@ShayanJavadi
Copy link
Contributor Author

@ShayanJavadi ShayanJavadi commented Feb 7, 2018

@timomeh I've update the pr, implemented the Badge component, and updated the documentation to reflect the changes. Please review my changes and let me know if you have any feedback.

Here's screenshots of the badge:

screen shot 2018-02-07 at 1 06 42 am

screen shot 2018-02-07 at 1 06 47 am

removed extra dependencies
Copy link
Owner

@timomeh timomeh left a comment

Thank you for your PR, I love it! ❤️

Unfortunately it doesn't render correctly in Android, you can only see a quarter of the badge. This is because in Android everything is overflow: hidden, and the container of the Tab icon cuts the badge.

Fortunately, you wrote pretty good code, so it's really easy to fix it and make it work in Android. I tried it locally and – instead of let you figure it out yourself – I commented the changes you need to do in order to support Android.

render() {
const { children, text, isVisible } = this.props;
const styles = this.createStyles();
if (!isVisible) {

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Remove the isVisible check, lines 70-72. The check is already done in Tab.js

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

So the check badgeText === undefined && !isBadgeVisible is to prevent the badge from rendering when there is no text. The check in here is for the user to be able to hide a badge even when there is text. We should keep this check

This comment has been minimized.

@timomeh

timomeh Feb 11, 2018
Owner

Ok, I see.

}

return (
<View style={{ flexDirection: "row" }}>

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Remove the wrapping <View style={{ flexDirection: "row" }}>.

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

Done


return (
<View style={{ flexDirection: "row" }}>
{children ? children : null}

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: children (which is the Tab Text) will be rendered in Tab.js, so remove this line

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

Done

const container = Object.assign(
{},
{
position: "relative",

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Change to position: 'absolute'

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

And to make the position: absolute work, add position: 'relative' to Tab.js StyleSheets.container. Somewhere here:

https://github.com/ShayanJavadi/react-native-material-bottom-navigation/blob/3f70221a611029b13f416f6ab51c2de569679d40/lib/Tab.js#L295-L304

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done

justifyContent: "center",
backgroundColor: "#F50057",
zIndex: 9999,
bottom: 5,

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Change bottom and right to something so it's correctly positioned. It worked for me with

top: 3,
left: '50%',
marginLeft: 15

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done

lib/Tab.js Outdated
style={badgeStyle}
isVisible={isBadgeVisible}
>
{active && activeIcon ? activeIcon : icon}

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: The icon is rendered in Tab.js, so remove this line.

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done

lib/Tab.js Outdated
<View ref="_bnic" collapsable={false}>
{active && activeIcon ? activeIcon : icon}
{this._renderBadge()}

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Change back to {active && activeIcon ? activeIcon : icon}. Move {this._renderBadge()} to the render function, right above {this._renderIcon()}

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done. moved it below it since moving it above was causing problems on iOS.

style: {},
}

export default class RippleBackgroundTransition extends Component {

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

class RippleBackgroundTransition -> class Badge :)

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done

lib/Tab.js Outdated
const { active, icon, activeIcon, badgeText, badgeSize, badgeStyle, isBadgeVisible } = this.props;

if (badgeText === undefined && !isBadgeVisible) {
return active && activeIcon ? activeIcon : icon;

This comment has been minimized.

@timomeh

timomeh Feb 9, 2018
Owner

For android compatibility: Change to return null, so nothing gets rendered if the badge is invisible.

This comment has been minimized.

@ShayanJavadi

ShayanJavadi Feb 11, 2018
Author Contributor

done

@ShayanJavadi
Copy link
Contributor Author

@ShayanJavadi ShayanJavadi commented Feb 10, 2018

@timomeh You're welcome! Thank you for making this library! I will implement/test your requested changes and will update the pr soon (thanks for figuring out the android compatibility issues!).

android compatibility
android compatibility
@ShayanJavadi
Copy link
Contributor Author

@ShayanJavadi ShayanJavadi commented Feb 11, 2018

@timomeh PR has been updated with the requested changes. Here's screenshots from both iOS and Android:
screen shot 2018-02-11 at 2 30 29 am
screen shot 2018-02-11 at 2 30 35 am

@timomeh
Copy link
Owner

@timomeh timomeh commented Feb 11, 2018

Looks good to me! I'm going to merge it and make a now release.

@timomeh timomeh merged commit 689133d into timomeh:master Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.