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

React-native-web compatibility + typescript typings #73

Merged
merged 11 commits into from
Feb 23, 2018

Conversation

PeterKottas
Copy link
Contributor

Hi, this solves all the compatibility issues with react-native-web plus I've added typescript typings. Sorry for mixing both in one PR but it's fairly small piece of work so I figured it would save some time.

@@ -299,11 +299,12 @@ const styles = StyleSheet.create({
flex: 1,
alignItems: 'center',
paddingTop: 8,
paddingBottom: 10,
paddingBottom: (Platform.OS !== 'web' ? 10 : 0),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Web sets overflow hidden for all texts and there's not enough room to display it properly. This looks a bit odd but works

@timomeh
Copy link
Owner

timomeh commented Feb 23, 2018

Wow, that's totally awesome! Thank you for contributing. I highly appreciate both improvements.

Since you recently pushed a few commits, is it still a WIP or is it finished?

@PeterKottas
Copy link
Contributor Author

Cheers mate, nah, it's good to go now. I was just fixing some stuff up. I've put a comment into PR about it. BS Android is forces overflow hidden on web so I had to play around with fixing the label being cut off. All good now ;)

@PeterKottas
Copy link
Contributor Author

There's actually one small issue left. Apparently css transition is not supported by react-native. However, react-native-web forwards unknown props to dom so the result actually animates beautifully. The problem is you get an ugly warning in console about it. I wish this could be solved. I'll look into some ways of solving it but I don't see an obvious one right away. (this only happens on web so we're not breaking anything, just room for improvement)

paddingLeft: 12,
paddingRight: 12,
backgroundColor: 'transparent',
position: "relative",
...(Platform.OS !== 'web' ? {} : { transition: '0.5s all' }),
Copy link
Owner

Choose a reason for hiding this comment

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

Just a comment, no change request:

Using transition to animate flex isn't really performant. https://csstriggers.com/flex-grow has a good explanation why. On a mobile browser with a mediocre phone, this could result in a laggy animation. But for now it's a very simple addition and I can't think of a better way from the top of my head.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. Let's hope phone users will stick with mobile app then :D

I think to avoid the problem, it would probably have to be some width based approach which would definitely be harder to maintain or follow. I'll do some testing on a phone later on though just to check. I think by design, bottom bar should be 5 items or less, right? Wondering if that's "good enough" for performance. Or if it doesn't matter and it will have problems no matter the count of animated objects.

Copy link
Owner

Choose a reason for hiding this comment

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

A method to make very smooth animations on web is FLIP. Here's the original article by Paul Lewis: https://aerotwist.com/blog/flip-your-animations/

I think for the scope of this PR, using transition should be enough. Maybe anything else is a bit too crazy.

@timomeh
Copy link
Owner

timomeh commented Feb 23, 2018

I tested it and it works really well. To be honest, I'm really astonished how well it works. I didn't thought that. 😄 I also tested it on my Pixel 2, and the flex-transition looks very smooth.

As I accidentally wrote in #74, I won't release it immediately. I'll first want to add tests, lint everything, and setup a CI.

I will follow up once it's published.

In the meantime you should be able to use npm install git://github.com/timomeh/react-native-material-bottom-navigation.git

@timomeh timomeh merged commit a1d6534 into timomeh:master Feb 23, 2018
@PeterKottas
Copy link
Contributor Author

Awesome! Good news indeed. I also though this would be an uphill battle but it seems react-native-web is becoming quite mature. Other than dropping LayoutAnimation, there was hardly anything I needed to do. Good stuff! GL with the setup, I am ok for now. Will change it once done

@PeterKottas
Copy link
Contributor Author

Hey bud, one more thing. I guess you are mid release so I don't want to PR again. It's very small thing. I found a way to fix those anoying warning on web with transition. This works:
PeterKottas@577edcc
It's very small, I think it will be easier for you to commit it directly. If not I can PR. Let me know.

@PeterKottas
Copy link
Contributor Author

Some docs about this : necolas/react-native-web#707

@timomeh
Copy link
Owner

timomeh commented Feb 24, 2018

Thank you, I'll include it.

@PeterKottas
Copy link
Contributor Author

👍

@timomeh
Copy link
Owner

timomeh commented Feb 27, 2018

Released as version 0.9.0 🚀

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

Successfully merging this pull request may close these issues.

2 participants