PixelRatio.pixel() #5076

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
9 participants
@corbt
Contributor

corbt commented Jan 1, 2016

This implements #5073. It adds a static method PixelRatio.pixel() which returns the smallest drawable line width, primarily for use in styles.

It also updates the example apps to use the new function.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 1, 2016

By analyzing the blame information on this pull request, we identified @mkonicek, @spicyj and @vjeux to be potential reviewers.

By analyzing the blame information on this pull request, we identified @mkonicek, @spicyj and @vjeux to be potential reviewers.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 1, 2016

Contributor
Contributor

corbt commented Jan 1, 2016

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 1, 2016

Contributor

I love this! 1 / PixelRatio.get() is so unintuitive. @sahrens and @nicklockwood what do you think?

Contributor

vjeux commented Jan 1, 2016

I love this! 1 / PixelRatio.get() is so unintuitive. @sahrens and @nicklockwood what do you think?

Libraries/Utilities/PixelRatio.js
+ * ```
+ */
+ static pixel(): number {
+ return 1 / this.get();

This comment has been minimized.

@vjeux

vjeux Jan 1, 2016

Contributor

Please use PixelRatio.get() here. This way the following still works:

var pixel = PixelRatio.pixel;
pixel()
@vjeux

vjeux Jan 1, 2016

Contributor

Please use PixelRatio.get() here. This way the following still works:

var pixel = PixelRatio.pixel;
pixel()
@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 2, 2016

Contributor

I agree with the concept, but I'm not sure if "pixel" is an intuitive function name. How about:

PixelRatio.pixelsToPoints(pixelValue)
PixelRatio.pointsToPixels(pointsValue)

Edit: I guess this should be:

getLayoutSizeForPixelSize(pixelSize)

To be consistent with the existing function, although TBH I think that's a ludicrously verbose name for a JavaScript API.

Contributor

nicklockwood commented Jan 2, 2016

I agree with the concept, but I'm not sure if "pixel" is an intuitive function name. How about:

PixelRatio.pixelsToPoints(pixelValue)
PixelRatio.pointsToPixels(pointsValue)

Edit: I guess this should be:

getLayoutSizeForPixelSize(pixelSize)

To be consistent with the existing function, although TBH I think that's a ludicrously verbose name for a JavaScript API.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 2, 2016

@corbt updated the pull request.

@corbt updated the pull request.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 2, 2016

Contributor

Another option might be to implement this as a constant, e.g.

PixelRatio.ONE_PIXEL

This should be doable, since window.scale never actually changes during the lifetime of a React Native application.

Contributor

nicklockwood commented Jan 2, 2016

Another option might be to implement this as a constant, e.g.

PixelRatio.ONE_PIXEL

This should be doable, since window.scale never actually changes during the lifetime of a React Native application.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 2, 2016

Contributor

Ok @vjeux I've updated the function to call it on the class.

@nicklockwood adding pixelsToPoints and renaming getPixelSizeForLayoutSize -> pointsToPixels do seem like good changes for consistency. I still don't think PixelRatio.pixelsToPoints(1) is a particularly elegant way to express the common sentiment "I have a line that I want 1 pixel thick," although it's loads better than 1 / PixelRatio.get().

I like your suggestion of PixelRatio.ONE_PIXEL a lot though, as long as the value of ONE_PIXEL really never changes. I'm not sure that is something that will always be guaranteed though (what does hooking up your iPhone/Android to a projector do?)

Contributor

corbt commented Jan 2, 2016

Ok @vjeux I've updated the function to call it on the class.

@nicklockwood adding pixelsToPoints and renaming getPixelSizeForLayoutSize -> pointsToPixels do seem like good changes for consistency. I still don't think PixelRatio.pixelsToPoints(1) is a particularly elegant way to express the common sentiment "I have a line that I want 1 pixel thick," although it's loads better than 1 / PixelRatio.get().

I like your suggestion of PixelRatio.ONE_PIXEL a lot though, as long as the value of ONE_PIXEL really never changes. I'm not sure that is something that will always be guaranteed though (what does hooking up your iPhone/Android to a projector do?)

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 2, 2016

Member

I actually don't think we should encourage writing 1 / PixelRatio.get() everywhere. We use it in a handful of examples and over a hundred times in the FB codebase, but it's rarely what you actually want. You don't actually want the thinnest line the device can draw: if devices had screens 10 times as dense as our current ones, you wouldn't want all of the lines in your app to be 1/10 as thick.

Similarly, never see rules of around 0.1pt or less on paper, even though a 600dpi printer could technically print them. LaTeX, a typesetting system, makes rules with width 0.4pt by default.

I think a much more reasonable approach is to decide how thick you would want a line on a screen with infinite resolution – maybe 0.5pt (equivalent to 1 pixel on a 2x screen), which I believe is what iOS uses in its table views – and then choose the closest number of pixels to that, regardless of if PixelRatio.get() is 1 or 100. Maybe we need some smarts to prevent borderWidth: 0.5 from getting rounded to 0 on a low-resolution screen, but choosing an actual, stable size is more likely to yield good results in the long run.

Member

sophiebits commented Jan 2, 2016

I actually don't think we should encourage writing 1 / PixelRatio.get() everywhere. We use it in a handful of examples and over a hundred times in the FB codebase, but it's rarely what you actually want. You don't actually want the thinnest line the device can draw: if devices had screens 10 times as dense as our current ones, you wouldn't want all of the lines in your app to be 1/10 as thick.

Similarly, never see rules of around 0.1pt or less on paper, even though a 600dpi printer could technically print them. LaTeX, a typesetting system, makes rules with width 0.4pt by default.

I think a much more reasonable approach is to decide how thick you would want a line on a screen with infinite resolution – maybe 0.5pt (equivalent to 1 pixel on a 2x screen), which I believe is what iOS uses in its table views – and then choose the closest number of pixels to that, regardless of if PixelRatio.get() is 1 or 100. Maybe we need some smarts to prevent borderWidth: 0.5 from getting rounded to 0 on a low-resolution screen, but choosing an actual, stable size is more likely to yield good results in the long run.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 2, 2016

Contributor

@spicyj makes a great point. It would be great to figure out how to stop using this hack coming from iOS and instead provide a proper way to specify a line that is of ~0.5 pixel height.

Contributor

vjeux commented Jan 2, 2016

@spicyj makes a great point. It would be great to figure out how to stop using this hack coming from iOS and instead provide a proper way to specify a line that is of ~0.5 pixel height.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 2, 2016

Contributor

We should probably just add a roundToNearestPixel() function then.

We already have the equivalent on the native side, and use it extensively in our layout logic, but we made the decision not to apply it automatically to borders IIRC, because it can dramatically affect the appearance (if someone actually prefers a fuzzy antialiased 1.5px wide border instead of a crisp 1 or 2px border, who are we to argue?)

Contributor

nicklockwood commented Jan 2, 2016

We should probably just add a roundToNearestPixel() function then.

We already have the equivalent on the native side, and use it extensively in our layout logic, but we made the decision not to apply it automatically to borders IIRC, because it can dramatically affect the appearance (if someone actually prefers a fuzzy antialiased 1.5px wide border instead of a crisp 1 or 2px border, who are we to argue?)

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 2, 2016

Contributor

@spicyj FWIW, I think you're mistaken about how iOS tableview key-lines work. I believe they're actually of different thickness depending on the device (0.5 points on an iPhone 6 with 2x Retina, and 0.33 points on an iPhone 6 plus with 3x Retina*).

Which doesn't make your central point wrong at all, but if people are seeking to emulate standard iOS L&F then using single-pixel keylines is the platform standard, even if it's not a particularly sensible decision on Apple's part.

(*although that doesn't actually map to an exact pixel size on the 6 plus anyway, since the entire screen is down-sampled to 1080p from it's simulated 3x resolution.)

Contributor

nicklockwood commented Jan 2, 2016

@spicyj FWIW, I think you're mistaken about how iOS tableview key-lines work. I believe they're actually of different thickness depending on the device (0.5 points on an iPhone 6 with 2x Retina, and 0.33 points on an iPhone 6 plus with 3x Retina*).

Which doesn't make your central point wrong at all, but if people are seeking to emulate standard iOS L&F then using single-pixel keylines is the platform standard, even if it's not a particularly sensible decision on Apple's part.

(*although that doesn't actually map to an exact pixel size on the 6 plus anyway, since the entire screen is down-sampled to 1080p from it's simulated 3x resolution.)

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 2, 2016

Contributor

Ok, @spicyj makes a fair point that the line thickness you want may not actually be 1px, especially with increasing screen densities.

However, I do think that there's a benefit to defining what a "thin line" is at the framework level, for a few reasons:

  1. It will tend towards UI standardization. Thin lines will always look the same, and ideally look the same as what native code on the platform uses for thin lines, eg what iOS uses for tableview key-lines.
  2. If there's any janky logic in deciding exactly what "a thin line" should resolve to on a particular platform/screen density, like what @nicklockwood was mentioning above about the line thickness on an iPhone 5 vs 6S, that logic just happens once in this function and end users don't have to worry about testing their line-thickness logic on every platform.
  3. The developer's intent (I want this to be a thin line) is clearer and easier to remember with a dedicated function/constant than by copy/pasting the math to get to it. Compare borderWidth: PixelRatio.roundToNearestPixel(0.5) to borderWidth: PixelRatio.HAIRLINE_WIDTH.
  4. Without this function developers may be tempted to just put borderWidth: 0.5, which will look fine on some devices/simulators and be antialiased and blurry on others, or in the worst case not even be visible on some very low screen densities.

I understand that adding a special case just for a single line may feel like it's increasing the API's size unnecessarily, but given how frequently you want a thin line, I think it justifies adding the extra function/constant. I would suspect that "making a really skinny line" would be the 80%+ use case of a hypothetical PixelRatio.roundToNearestPixel anyway, and that it justifies its own special case to make sure it's easy for developers to get right.

Contributor

corbt commented Jan 2, 2016

Ok, @spicyj makes a fair point that the line thickness you want may not actually be 1px, especially with increasing screen densities.

However, I do think that there's a benefit to defining what a "thin line" is at the framework level, for a few reasons:

  1. It will tend towards UI standardization. Thin lines will always look the same, and ideally look the same as what native code on the platform uses for thin lines, eg what iOS uses for tableview key-lines.
  2. If there's any janky logic in deciding exactly what "a thin line" should resolve to on a particular platform/screen density, like what @nicklockwood was mentioning above about the line thickness on an iPhone 5 vs 6S, that logic just happens once in this function and end users don't have to worry about testing their line-thickness logic on every platform.
  3. The developer's intent (I want this to be a thin line) is clearer and easier to remember with a dedicated function/constant than by copy/pasting the math to get to it. Compare borderWidth: PixelRatio.roundToNearestPixel(0.5) to borderWidth: PixelRatio.HAIRLINE_WIDTH.
  4. Without this function developers may be tempted to just put borderWidth: 0.5, which will look fine on some devices/simulators and be antialiased and blurry on others, or in the worst case not even be visible on some very low screen densities.

I understand that adding a special case just for a single line may feel like it's increasing the API's size unnecessarily, but given how frequently you want a thin line, I think it justifies adding the extra function/constant. I would suspect that "making a really skinny line" would be the 80%+ use case of a hypothetical PixelRatio.roundToNearestPixel anyway, and that it justifies its own special case to make sure it's easy for developers to get right.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 2, 2016

Contributor

Actually since a consistent thin-line-width is mostly a stylistic concern and only tangentially related to the PixelRatio, it probably shouldn't live on PixelRatio anymore. Not sure where the best place would be, maybe StyleSheet.HAIRLINE_WIDTH?

Contributor

corbt commented Jan 2, 2016

Actually since a consistent thin-line-width is mostly a stylistic concern and only tangentially related to the PixelRatio, it probably shouldn't live on PixelRatio anymore. Not sure where the best place would be, maybe StyleSheet.HAIRLINE_WIDTH?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 2, 2016

Member

@nicklockwood But round(0.4pt) (or any number between 0.25pt and 0.5pt) would also give those numbers and makes a lot more sense in my mind.

@corbt Defining a constant that can be shared makes sense to me.

Member

sophiebits commented Jan 2, 2016

@nicklockwood But round(0.4pt) (or any number between 0.25pt and 0.5pt) would also give those numbers and makes a lot more sense in my mind.

@corbt Defining a constant that can be shared makes sense to me.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 4, 2016

@corbt updated the pull request.

@corbt updated the pull request.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 4, 2016

Contributor

Ok, I've rewritten this PR informed by the discussion above. It now has 3 parts to it:

  1. Added a PixelRatio.roundToNearestPixel function
  2. Added a StyleSheet.Constants.HAIRLINE_WIDTH "constant" (it'll be constant within the context of a running app, although it can vary with different platforms/screen densities). This is currently defined as PixelRatio.roundToNearestPixel(0.4). I also had to refactor the StyleSheet module a bit to make this work and make the generated documentation show up.
  3. Updated the examples to use the new constant instead of the 1 / PixelRatio.get() hack.
Contributor

corbt commented Jan 4, 2016

Ok, I've rewritten this PR informed by the discussion above. It now has 3 parts to it:

  1. Added a PixelRatio.roundToNearestPixel function
  2. Added a StyleSheet.Constants.HAIRLINE_WIDTH "constant" (it'll be constant within the context of a running app, although it can vary with different platforms/screen densities). This is currently defined as PixelRatio.roundToNearestPixel(0.4). I also had to refactor the StyleSheet module a bit to make this work and make the generated documentation show up.
  3. Updated the examples to use the new constant instead of the 1 / PixelRatio.get() hack.
Libraries/StyleSheet/StyleSheet.js
-module.exports = StyleSheet;
+ /**
+ * Creates a StyleSheet object from the .

This comment has been minimized.

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Incomplete comment

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Incomplete comment

Libraries/StyleSheet/StyleSheet.js
+ * }
+ * ```
+ *
+ * This constant will always be a round number of pixels (so a line

This comment has been minimized.

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Something wrong with the indenting here

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Something wrong with the indenting here

Libraries/StyleSheet/StyleSheet.js
+ result[key] = StyleSheetRegistry.registerStyle(obj[key]);
+ }
+ return result;
+ }

This comment has been minimized.

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Misaligned braces

@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Misaligned braces

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 4, 2016

Contributor

Looks good to me. Some formatting issues in StyleSheet.js though.

Contributor

nicklockwood commented Jan 4, 2016

Looks good to me. Some formatting issues in StyleSheet.js though.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 4, 2016

@corbt updated the pull request.

@corbt updated the pull request.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 4, 2016

Contributor

Ok, fixed indentation issues in StyleSheet.js. Good catch.

Contributor

corbt commented Jan 4, 2016

Ok, fixed indentation issues in StyleSheet.js. Good catch.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
Contributor

nicklockwood commented Jan 4, 2016

👍 @facebook-github-bot shipit

Libraries/StyleSheet/StyleSheet.js
+
+/* TODO(brentvatne) docs are needed for this */
+var flatten = require('flattenStyle');
+var PixelRatio = require('PixelRatio');

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

The convention is to have a block with all the letters that start with a capital and sorted a-z, an empty line and then all the lowercase.

var PixelRatio = require('PixelRatio');
var StyleSheetRegistry = require('StyleSheetRegistry');
var StyleSheetRegistry = require('StyleSheetRegistry');

var flatten = require('flattenStyle');
@vjeux

vjeux Jan 4, 2016

Contributor

The convention is to have a block with all the letters that start with a capital and sorted a-z, an empty line and then all the lowercase.

var PixelRatio = require('PixelRatio');
var StyleSheetRegistry = require('StyleSheetRegistry');
var StyleSheetRegistry = require('StyleSheetRegistry');

var flatten = require('flattenStyle');
Libraries/StyleSheet/StyleSheet.js
+var flatten = require('flattenStyle');
+var PixelRatio = require('PixelRatio');
+
+// Used to compute the constant Stylesheet.Constants.HAIRLINE_WIDTH

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

cc @sebmarkbage this is going to break you

@vjeux

vjeux Jan 4, 2016

Contributor

cc @sebmarkbage this is going to break you

This comment has been minimized.

@sebmarkbage

sebmarkbage Jan 4, 2016

Member

Unfortunate since in the ideal API this would be responsive, but I'll find a workaround.

@sebmarkbage

sebmarkbage Jan 4, 2016

Member

Unfortunate since in the ideal API this would be responsive, but I'll find a workaround.

Libraries/StyleSheet/StyleSheet.js
@@ -13,7 +13,16 @@
var StyleSheetRegistry = require('StyleSheetRegistry');
var StyleSheetValidation = require('StyleSheetValidation');
-var flattenStyle = require('flattenStyle');
+
+/* TODO(brentvatne) docs are needed for this */

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

Can you avoid adding comments like this inside the code. If those will be documented, they will be inside of those functions and this comment will unlikely be removed and be misleading.

@vjeux

vjeux Jan 4, 2016

Contributor

Can you avoid adding comments like this inside the code. If those will be documented, they will be inside of those functions and this comment will unlikely be removed and be misleading.

This comment has been minimized.

@corbt

corbt Jan 4, 2016

Contributor

Sure. That comment was already there on the flatten export, I was just trying to find a better place to put it that wouldn't show up in the docs.

@corbt

corbt Jan 4, 2016

Contributor

Sure. That comment was already there on the flatten export, I was just trying to find a better place to put it that wouldn't show up in the docs.

Libraries/StyleSheet/StyleSheet.js
+ * not rely on it being a constant size, because on different platforms
+ * and screen densities its value may be calculated differently.
+ */
+ Constants: {

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

Can you remove this extra level of indirection. We try to avoid unnecessary nesting as much as possible.

StyleSheet.hairlineWidth
@vjeux

vjeux Jan 4, 2016

Contributor

Can you remove this extra level of indirection. We try to avoid unnecessary nesting as much as possible.

StyleSheet.hairlineWidth
Libraries/StyleSheet/StyleSheet.js
+ * and screen densities its value may be calculated differently.
+ */
+ Constants: {
+ HAIRLINE_WIDTH: hairlineWidth

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

For constants, we use camelCase:

Image.resizeMode.contain
@vjeux

vjeux Jan 4, 2016

Contributor

For constants, we use camelCase:

Image.resizeMode.contain
+ * of 3, `PixelRatio.roundToNearestPixel(8.4) = 8.33`, which corresponds to
+ * exactly (8.33 * 3) = 25 pixels.
+ */
+ static roundToNearestPixel(layoutSize: number): number {

This comment has been minimized.

@vjeux

vjeux Jan 4, 2016

Contributor

Me gusta, this can also be useful for figuring out image sizes.

@vjeux

vjeux Jan 4, 2016

Contributor

Me gusta, this can also be useful for figuring out image sizes.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 4, 2016

Contributor

Thanks a lot for doing that, I'm really happy with that constant :)

Contributor

vjeux commented Jan 4, 2016

Thanks a lot for doing that, I'm really happy with that constant :)

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 4, 2016

Member

It is quite unfortunate that this API assumes that the device pixel ratio is constant for the device and constant for all screens of the device. This means that this will likely not nicely transfer to the desktop where we want to reactively change the ratio as a window moves between screens.

Doesn't iOS support different pixel ratios for external displays over AirPlay too? Android supports multiple screens too, right?

It would be nicer if this was passed through the React context so that a component's position in the tree determined its pixel ratio. Unfortunately React's context API is kind of sucky atm but maybe we can make it a mixin for now?

EDIT: It doesn't even have to be implemented yet but it would be nice if the API was future proof. For example, requiring a component to be passed as the first argument. E.g. PixelRatio.get(this) and PixelRatio.roundToNearestPixel(this, 12.3) maybe? I understand that this is kind of awkward when most styles are extracted to the bottom of the file.

Member

sebmarkbage commented Jan 4, 2016

It is quite unfortunate that this API assumes that the device pixel ratio is constant for the device and constant for all screens of the device. This means that this will likely not nicely transfer to the desktop where we want to reactively change the ratio as a window moves between screens.

Doesn't iOS support different pixel ratios for external displays over AirPlay too? Android supports multiple screens too, right?

It would be nicer if this was passed through the React context so that a component's position in the tree determined its pixel ratio. Unfortunately React's context API is kind of sucky atm but maybe we can make it a mixin for now?

EDIT: It doesn't even have to be implemented yet but it would be nice if the API was future proof. For example, requiring a component to be passed as the first argument. E.g. PixelRatio.get(this) and PixelRatio.roundToNearestPixel(this, 12.3) maybe? I understand that this is kind of awkward when most styles are extracted to the bottom of the file.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 4, 2016

Contributor

@sebmarkbage hmm, that's a good point. It's an edge case since very few users have multiple screens of different pixel densities, but it is a design flaw nonetheless.

Contributor

nicklockwood commented Jan 4, 2016

@sebmarkbage hmm, that's a good point. It's an edge case since very few users have multiple screens of different pixel densities, but it is a design flaw nonetheless.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 4, 2016

Contributor

@sebmarkbage by the way, this is an issue with existing code as well, there's 128 calls of 1 / PixelRatio.get() most of them in StyleSheet.create()

Contributor

vjeux commented Jan 4, 2016

@sebmarkbage by the way, this is an issue with existing code as well, there's 128 calls of 1 / PixelRatio.get() most of them in StyleSheet.create()

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 4, 2016

Member

We could also allow a place holder value in style sheets. Kind of like 1cm or 1pt in CSS. Like PixelRatio.get() could return { value: 1, type: 'pixel-ratio' }. The hope for JS value types is that they will be able to model CSS values with units associated as well as overload operators.

If we assume that we'll eventually have these types with overloadable operators, most of these operations can probably keep working as is with a placeholder value.

E.g. 1 / PixelRatio.get() could become something like new PixelRatioValue(divide(new PixelRatioValue(1), PixelRatio.get())) semantically.

That's probably a much nicer API. We could live with this failing in edge cases for now, knowing that we have an upgrade path if we can replace the numbers with placeholder values in the future.

Member

sebmarkbage commented Jan 4, 2016

We could also allow a place holder value in style sheets. Kind of like 1cm or 1pt in CSS. Like PixelRatio.get() could return { value: 1, type: 'pixel-ratio' }. The hope for JS value types is that they will be able to model CSS values with units associated as well as overload operators.

If we assume that we'll eventually have these types with overloadable operators, most of these operations can probably keep working as is with a placeholder value.

E.g. 1 / PixelRatio.get() could become something like new PixelRatioValue(divide(new PixelRatioValue(1), PixelRatio.get())) semantically.

That's probably a much nicer API. We could live with this failing in edge cases for now, knowing that we have an upgrade path if we can replace the numbers with placeholder values in the future.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 4, 2016

Contributor

@corbt can you fix my suggestions and then we'll land it, thanks!

Contributor

vjeux commented Jan 4, 2016

@corbt can you fix my suggestions and then we'll land it, thanks!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 4, 2016

@corbt updated the pull request.

@corbt updated the pull request.

@vjeux

This comment has been minimized.

Show comment
Hide comment
Contributor

vjeux commented Jan 4, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 4, 2016

Contributor

I've updated the PR to match @vjeux's suggestions.

However, I'm also in favor of changing StyleSheet.hairlineWidth to a function instead of a constant. It seems likely that the actual dp value of what a "thin line" means may change at runtime as users use different screens, connect to projectors, etc, and by making it a function it would be easier to add an argument in the future (a react context or whatever makes sense) that could be used to update the thickness on the fly, while maintaining backwards compatibility by using the current implementation if no argument is passed in.

Contributor

corbt commented Jan 4, 2016

I've updated the PR to match @vjeux's suggestions.

However, I'm also in favor of changing StyleSheet.hairlineWidth to a function instead of a constant. It seems likely that the actual dp value of what a "thin line" means may change at runtime as users use different screens, connect to projectors, etc, and by making it a function it would be easier to add an argument in the future (a react context or whatever makes sense) that could be used to update the thickness on the fly, while maintaining backwards compatibility by using the current implementation if no argument is passed in.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 4, 2016

Member

@corbt That would certainly help my use case a lot but I'm not sure it matters much if it is commonly used in static StyleSheet.create() calls anyway. You risk inconsistent behavior instead.

Member

sebmarkbage commented Jan 4, 2016

@corbt That would certainly help my use case a lot but I'm not sure it matters much if it is commonly used in static StyleSheet.create() calls anyway. You risk inconsistent behavior instead.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 4, 2016

Contributor

Happy if you make it getHairlineWidth()

Let me know if you are doing that, so that I stop the current land

Contributor

vjeux commented Jan 4, 2016

Happy if you make it getHairlineWidth()

Let me know if you are doing that, so that I stop the current land

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 4, 2016

Contributor

@vjeux well like @sebmarkbage pointed out this will predominately be used in precomputed StyleSheets anyway. The right place to make this value responsive might have to be in the layout internals instead of in the stylesheet, in which case a constant in the stylesheet will still work fine.

So anyway, I'm fine with just shipping this and in the worst case having to deprecate the constant in the future if RN figures out a better way to calculate the right value on the fly.

Contributor

corbt commented Jan 4, 2016

@vjeux well like @sebmarkbage pointed out this will predominately be used in precomputed StyleSheets anyway. The right place to make this value responsive might have to be in the layout internals instead of in the stylesheet, in which case a constant in the stylesheet will still work fine.

So anyway, I'm fine with just shipping this and in the worst case having to deprecate the constant in the future if RN figures out a better way to calculate the right value on the fly.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 4, 2016

Member

Tangent: I bet there is a nice way of modeling layout and rendering where these units will automatically work themselves out. It's just not the CSS box model. Something to think about. :)

Member

sebmarkbage commented Jan 4, 2016

Tangent: I bet there is a nice way of modeling layout and rendering where these units will automatically work themselves out. It's just not the CSS box model. Something to think about. :)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jan 4, 2016

Contributor

Sorry about that, the current version it is based on isn't passing on our internal trunk. Do you mind rebasing?

Contributor

vjeux commented Jan 4, 2016

Sorry about that, the current version it is based on isn't passing on our internal trunk. Do you mind rebasing?

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 4, 2016

Contributor

@vjeux rebased

Contributor

corbt commented Jan 4, 2016

@vjeux rebased

@vjeux

This comment has been minimized.

Show comment
Hide comment
Contributor

vjeux commented Jan 4, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 6, 2016

Contributor

Failed because of Flow errors, trying again.

Contributor

mkonicek commented Jan 6, 2016

Failed because of Flow errors, trying again.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Jan 8, 2016

Contributor

@mkonicek any progress on this? I've checked locally and don't see any errors with flow, but if you tell me how to run it such that they appear I'm happy to update the PR to fix it.

Contributor

corbt commented Jan 8, 2016

@mkonicek any progress on this? I've checked locally and don't see any errors with flow, but if you tell me how to run it such that they appear I'm happy to update the PR to fix it.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Jan 11, 2016

Contributor

@corbt, some internal tests got broken because Stylesheet introduced a new dependency PixelRatio.
I'll fix it and it will get merged in a couple of hours.
Sorry for the wait.

Contributor

bestander commented Jan 11, 2016

@corbt, some internal tests got broken because Stylesheet introduced a new dependency PixelRatio.
I'll fix it and it will get merged in a couple of hours.
Sorry for the wait.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Jan 11, 2016

Contributor

some more problems with CI system we face, please hold on

Contributor

bestander commented Jan 11, 2016

some more problems with CI system we face, please hold on

@ghost ghost closed this in cd89016 Jan 15, 2016

christopherdro added a commit to wildlifela/react-native that referenced this pull request Jan 20, 2016

PixelRatio.pixel()
Summary:
This implements #5073. It adds a static method `PixelRatio.pixel()` which returns the smallest drawable line width, primarily for use in styles.

It also updates the example apps to use the new function.
Closes facebook#5076

Reviewed By: svcscm

Differential Revision: D2799849

Pulled By: nicklockwood

fb-gh-sync-id: b83a77790601fe882affbf65531114e7c5cf4bdf

@corbt corbt deleted the corbt:pixelratio-pixel branch Feb 3, 2016

cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017

PixelRatio.pixel()
Summary:
This implements #5073. It adds a static method `PixelRatio.pixel()` which returns the smallest drawable line width, primarily for use in styles.

It also updates the example apps to use the new function.
Closes facebook/react-native#5076

Reviewed By: svcscm

Differential Revision: D2799849

Pulled By: nicklockwood

fb-gh-sync-id: b83a77790601fe882affbf65531114e7c5cf4bdf

This issue was closed.

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