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

Why round? #17

Closed
Kerumen opened this issue Jun 17, 2016 · 14 comments
Closed

Why round? #17

Kerumen opened this issue Jun 17, 2016 · 14 comments

Comments

@Kerumen
Copy link
Contributor

Kerumen commented Jun 17, 2016

I was wondering why round all calculated variables?

I blocked on this because I have a borderWidth variable of 0.5 but it gets rounded to 1 and it's frustrating..

React Native already round the style for pixel snapping so it seems unneeded.

@vitalets
Copy link
Owner

vitalets commented Jul 3, 2016

hi @Kerumen

thanks for the info! I agree rounding is not needed. Could you make a pr?

@pampang
Copy link

pampang commented Jan 10, 2017

Without rounding, how can I fixed lineHeight which needs its value to be an integer?

@vitalets
Copy link
Owner

@pampang does not RN do it itself?

@pampang
Copy link

pampang commented Jan 10, 2017

@vitalets RN just alert an error, and that's all.
I've make it right by using my code like below:
lineHeight: Math.round(EStyleSheet.value('1.8rem'))

@vitalets
Copy link
Owner

Ok, got it.
So it seems we should revert this change and allow RN to round when needed.

@Kerumen did you face such problems?

@Kerumen
Copy link
Contributor Author

Kerumen commented Jan 10, 2017

@vitalets No, never. I just tried with RN 0.39.2 and a float line height, I don't have any warning.

I don't think we should revert this change because at the moment the user can manually round as @pampang did. But if the library round, there is no way for the user to prevent this. I guess it's better in the way it is now.

@vitalets
Copy link
Owner

@pampang could you screenshot an alert?

@pampang
Copy link

pampang commented Jan 11, 2017

@vitalets
This issue is discussed in facebook/react-native#7877 for a while.
I am using RN 0.37.0. Maybe it is a bug that fixed by RN 0.39.1.

@Kerumen
Copy link
Contributor Author

Kerumen commented Jan 11, 2017

Sorry I tested only on iOS. This seems to happen on Android.

@egormerkushev
Copy link

egormerkushev commented Apr 11, 2017

So, what is proper way to round line heights with EStyleSheet for Android?

I found this solution:

theme = {
  $baseLineHeight: 1.4,
  $h5FontSize: 12,
}

EStyleSheet.build(theme);

const text = {
  tileText: {
    fontSize: '$h5FontSize',
    lineHeight: () => { return Math.round(EStyleSheet.value('$baseLineHeight*$h5FontSize')) },
  }
}

let textStyles = EStyleSheet.create(text)

@vitalets
Copy link
Owner

hi @egormerkushev !
RN rounds pixel values automatically.
Do you have any warnings if you don't round manually?

  tileText: {
    ...
    lineHeight: '$baseLineHeight*$h5FontSize',
  }

@egormerkushev
Copy link

@vitalets I use RN 0.42 and using not whole values for line heights causes a crash on Android 6.0 for example.

@vitalets
Copy link
Owner

Yes, by facebook/react-native#7877 (comment) lineHeight property in RN must be integer. And throws error if not.
I also think it's inconsistent with other styles (that just round values).

So especially for lineHeight we should round manually as you done.

@egormerkushev
Copy link

egormerkushev commented Apr 12, 2017

@vitalets yep. I upvoted for this feature request yesterday. If my solution is not bad - may be it will be useful for other developers. ✌🏻

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

No branches or pull requests

4 participants