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

[View] Keep view's aspect ratio based on new prop #1538

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
@paramaggarwal
Contributor

paramaggarwal commented Jun 6, 2015

Continues from #858.

Current behaviour:

  1. When a static image is configured with <Image source=require("image_name.jpg") />, the packager adds the width and height into the source prop. Hence there is no need of setting dimensions in style.
  2. But for remote images, this width and height is forcefully set on the style prop, which means if one uses only alignSelf: 'stretch' to layout the image, the height would not change in the same ratio as the width inherited from the parent.

This pull request fixes this. I believe the basic problem that this attempts to solve is very genuine. It includes a measure function on NetworkImageView to maintain aspect ratio of image when dimensions are known beforehand.

Now just like automatic sizing and layout of text, the same would work for images that actually take the entire width of the device, and the height would be configured automatically based on the calculated aspect ratio of the image:

<View style={{flex: 1, paddingTop: 20}}>
  <Image
    style={{margin: 5}}
    source={{
      uri: "http://placehold.it/800x100",
      height: 100,
      width: 800
    }}
  />
  <Image
    style={{margin: 5}}
    source={{
      uri: "http://placehold.it/600x300",
      height: 300,
      width: 600
    }}
  />
  <Image
    style={{margin: 5}}
    source={{
      uri: "http://placehold.it/400x400",
      height: 400,
      width: 400
    }}
  />
</View>

Screenshots comparing the output of above code on devices with different widths.

Open to suggestions on how to improve this pull request.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 7, 2015

Collaborator

@paramaggarwal - this is a really interesting idea! cc @vjeux

An initial bit of feedback:

  • I'm not sure that this is best suited as an addition to Image, but rather as a separate component.
  • It feels awkward to specify the width and height in the style when that will not be respected - it would make more sense to include this in the source prop sorry I read that wrong 😄
Collaborator

brentvatne commented Jun 7, 2015

@paramaggarwal - this is a really interesting idea! cc @vjeux

An initial bit of feedback:

  • I'm not sure that this is best suited as an addition to Image, but rather as a separate component.
  • It feels awkward to specify the width and height in the style when that will not be respected - it would make more sense to include this in the source prop sorry I read that wrong 😄
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 7, 2015

Collaborator

I think we want this in core since it achieves parity between static and network images and it's a fundamental that applies to all images. To be clear, @paramaggarwal's proposal maintains the current invariant of needing to specify the image size up front (before we've downloaded it from the network). The sole difference is that it's the intrinsic image size, not the displayed size (in the style prop) that needs to be specified. This provides a very consistent API for users that I could explain to people:

  • You should specify an image's intrinsic dimensions in the <Image source> prop
  • The intrinsic dimensions, which tell us the intrinsic aspect ratio, will be used to help the layout engine calculate the image's displayed dimensions (e.g. the flex example in this discussion)
  • If you don't specify intrinsic dimensions then you must specify the image's displayed dimensions via width and height in the style prop.
  • The packager automatically specifies the intrinsic dimensions when it replaces require('image!icon.png')
  • Missing intrinsic dimensions default to zero

@paramaggarwal: might want to rename original width/height to intrinsic width/height to match w3c docs.

Collaborator

ide commented Jun 7, 2015

I think we want this in core since it achieves parity between static and network images and it's a fundamental that applies to all images. To be clear, @paramaggarwal's proposal maintains the current invariant of needing to specify the image size up front (before we've downloaded it from the network). The sole difference is that it's the intrinsic image size, not the displayed size (in the style prop) that needs to be specified. This provides a very consistent API for users that I could explain to people:

  • You should specify an image's intrinsic dimensions in the <Image source> prop
  • The intrinsic dimensions, which tell us the intrinsic aspect ratio, will be used to help the layout engine calculate the image's displayed dimensions (e.g. the flex example in this discussion)
  • If you don't specify intrinsic dimensions then you must specify the image's displayed dimensions via width and height in the style prop.
  • The packager automatically specifies the intrinsic dimensions when it replaces require('image!icon.png')
  • Missing intrinsic dimensions default to zero

@paramaggarwal: might want to rename original width/height to intrinsic width/height to match w3c docs.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jun 8, 2015

Collaborator

Great points @ide - what are your thoughts on this @vjeux? I know you've looked into this kind of thing previously.

Collaborator

brentvatne commented Jun 8, 2015

Great points @ide - what are your thoughts on this @vjeux? I know you've looked into this kind of thing previously.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 8, 2015

Contributor

Thanks @brentvatne and @ide. I have renamed originalHeight/originalWidth to intrinsicHeight/intrinsicWidth. I have also updated the calculated dimensions to be 0x0 when intrinsic dimensions are not provided instead of the previous assumption of a default aspectRatio of 4:3.

For the benefit of others, here is the flow of how the final dimensions of a network image are calculated in order of priority:

  1. Absolute height and width specified in style.
  2. Flex calculated height and width coming from style.
  3. Intrinsic height and width used from the dimensions passed to source.

In all of these scenarios, if it is not able to calculate the height from the given info, it will attempt to use the aspect ratio computed from the dimensions passed to source.

Travis test is failing because of this failure on TEST_TYPE=build_website over here, and not in the obj-c, js tests.

{ [Error: socket hang up] code: 'ECONNRESET' }

Also, should I extend this change to static images also? So that even static images would have the same behaviour in terms of calculating their height themselves given the intrinsic dimentions in source.

Contributor

paramaggarwal commented Jun 8, 2015

Thanks @brentvatne and @ide. I have renamed originalHeight/originalWidth to intrinsicHeight/intrinsicWidth. I have also updated the calculated dimensions to be 0x0 when intrinsic dimensions are not provided instead of the previous assumption of a default aspectRatio of 4:3.

For the benefit of others, here is the flow of how the final dimensions of a network image are calculated in order of priority:

  1. Absolute height and width specified in style.
  2. Flex calculated height and width coming from style.
  3. Intrinsic height and width used from the dimensions passed to source.

In all of these scenarios, if it is not able to calculate the height from the given info, it will attempt to use the aspect ratio computed from the dimensions passed to source.

Travis test is failing because of this failure on TEST_TYPE=build_website over here, and not in the obj-c, js tests.

{ [Error: socket hang up] code: 'ECONNRESET' }

Also, should I extend this change to static images also? So that even static images would have the same behaviour in terms of calculating their height themselves given the intrinsic dimentions in source.

Show outdated Hide outdated Libraries/Image/Image.ios.js
var style = flattenStyle([{width, height}, styles.base, this.props.style]);
if (isNetwork) {
// NetworkImageView uses a measure method instead to set original width and height
style = flattenStyle([styles.base, this.props.style]);

This comment has been minimized.

@sahrens

sahrens Jun 13, 2015

Contributor

you're throwing away the work done in flattenStyle above - it can be quite expensive, so only do one or the other.

@sahrens

sahrens Jun 13, 2015

Contributor

you're throwing away the work done in flattenStyle above - it can be quite expensive, so only do one or the other.

This comment has been minimized.

@paramaggarwal

paramaggarwal Jun 13, 2015

Contributor

Good one, I fixed this.

@paramaggarwal

paramaggarwal Jun 13, 2015

Contributor

Good one, I fixed this.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 13, 2015

Contributor

This is cool, but we have some changes planned with the source width/height that may need to coordinate here (although I think this is a better solution to the problems we've been seeing than we were planning). Assigning to @frantic who has been working on that image stuff.

Contributor

sahrens commented Jun 13, 2015

This is cool, but we have some changes planned with the source width/height that may need to coordinate here (although I think this is a better solution to the problems we've been seeing than we were planning). Assigning to @frantic who has been working on that image stuff.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 13, 2015

Contributor

Also, can you rebase? It might also give a passing test run.

Contributor

sahrens commented Jun 13, 2015

Also, can you rebase? It might also give a passing test run.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 13, 2015

Contributor

@sahrens Squashed commits into one, rebased to fix merge conflicts, removed redundant flattenStyle and tests now passing!

Contributor

paramaggarwal commented Jun 13, 2015

@sahrens Squashed commits into one, rebased to fix merge conflicts, removed redundant flattenStyle and tests now passing!

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 16, 2015

Contributor

@frantic please share your thoughts.

Also, I noticed while using this, that setting a measure function on css_layout will not cause it to look for measurements based on child elements. So if you have an image with children, and you would like the image to get its dimension based on children, then this is a no-go.

Currently I do this:

- (void)fillCSSNode:(css_node_t *)node
{
  [super fillCSSNode:node];
  if (_intrinsicHeight && _intrinsicWidth) {
    node->measure = RCTMeasure;
  }
}

So this works well. It works like an opt-in switch. Passing in source.width and source.height will mean that you want the image to size itself based on its aspect ratio than its children. The problem is that if an image element was first configured without this, and then later on gets the source.width and source.height parameters, this aspect ratio behaviour would not get turned on. Thoughts?

Pinging @vjeux for thoughts regarding the use of css-layout measure here.

Contributor

paramaggarwal commented Jun 16, 2015

@frantic please share your thoughts.

Also, I noticed while using this, that setting a measure function on css_layout will not cause it to look for measurements based on child elements. So if you have an image with children, and you would like the image to get its dimension based on children, then this is a no-go.

Currently I do this:

- (void)fillCSSNode:(css_node_t *)node
{
  [super fillCSSNode:node];
  if (_intrinsicHeight && _intrinsicWidth) {
    node->measure = RCTMeasure;
  }
}

So this works well. It works like an opt-in switch. Passing in source.width and source.height will mean that you want the image to size itself based on its aspect ratio than its children. The problem is that if an image element was first configured without this, and then later on gets the source.width and source.height parameters, this aspect ratio behaviour would not get turned on. Thoughts?

Pinging @vjeux for thoughts regarding the use of css-layout measure here.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 16, 2015

Contributor

The problem is that if an image element was first configured without this, and then later on gets the source.width and source.height parameters, this aspect ratio behaviour would not get turned on.

In general, it's really important that the order of property changes not matter, just the final state - can you fix that?

Contributor

sahrens commented Jun 16, 2015

The problem is that if an image element was first configured without this, and then later on gets the source.width and source.height parameters, this aspect ratio behaviour would not get turned on.

In general, it's really important that the order of property changes not matter, just the final state - can you fix that?

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 17, 2015

Contributor

@sahrens, I thought of a way to do it conditionally - and updated the PR. Now only if an intrinsicHeight or intrinsicWidth are passed in, it will attempt to measure layout dynamically. So this is taken care of.

@frantic @vjeux please do share your thoughts.

Contributor

paramaggarwal commented Jun 17, 2015

@sahrens, I thought of a way to do it conditionally - and updated the PR. Now only if an intrinsicHeight or intrinsicWidth are passed in, it will attempt to measure layout dynamically. So this is taken care of.

@frantic @vjeux please do share your thoughts.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 17, 2015

Contributor

Sorry for the super long delay in my response.

Can we polyfill this on web somehow? Two ways would be padding-bottom: (aspectRatio * 100)% or using with a data uri that's a transparent image of the intrisic size.

If we can't then I'm going to push back on this. Otherwise this seems like a fine change. Very much opt-in.

For your measure question, this is indeed an issue, we probably need to change the caching logic to let a component defined function also tell if it should invalidate caching or not (based on intrisic width)

Contributor

vjeux commented Jun 17, 2015

Sorry for the super long delay in my response.

Can we polyfill this on web somehow? Two ways would be padding-bottom: (aspectRatio * 100)% or using with a data uri that's a transparent image of the intrisic size.

If we can't then I'm going to push back on this. Otherwise this seems like a fine change. Very much opt-in.

For your measure question, this is indeed an issue, we probably need to change the caching logic to let a component defined function also tell if it should invalidate caching or not (based on intrisic width)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 17, 2015

Contributor

Btw image themselves may not be the only one that require an aspect ratio. If you want to do a responsive square grid, you need to maintain 1:1 aspect ratio on a View. Out of the scope of this pull request but would be nice to unify the concept of aspect ratio constraint

Contributor

vjeux commented Jun 17, 2015

Btw image themselves may not be the only one that require an aspect ratio. If you want to do a responsive square grid, you need to maintain 1:1 aspect ratio on a View. Out of the scope of this pull request but would be nice to unify the concept of aspect ratio constraint

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 17, 2015

Contributor

Isn't this the default behaviour on the web already? When an <img> is used on the web, the browser fetches the image, calculates the width and height and maintains the aspect ratio of the image automatically. See this JSBin, try resizing the output panel and note that when the width of the image is changing, the height is also changing. Hence the aspect ratio of the image is automatically maintained by the browser.

Now in React Native, we do not load images and calculate dimensions, we expect the dimensions to be passed before-hand as part of style. But then we lose the capability of changing height of image in accordance with the width of the image.

This PR, helps us give the original intrinsicHeight and intrinsicWidth of the image (which a browser finds out after actually loading the image, while we provide beforehand) - and based on this, using the measure method of css-layout, we are able to maintain the aspect ratio of the image when it's width changes.

In much this way, the src attribute on the web, maps to the source attribute in React Native. Only that, in addition to the uri, we also need to give the width and height of the image - completely describing the remote resource.

<Image source={{
  uri: 'https://upload.wikimedia.org/wikipedia/commons/thumb/5/57/React.js_logo.svg/600px-React.js_logo.svg.png',
  width: 600,
  height: 600
}} />
Contributor

paramaggarwal commented Jun 17, 2015

Isn't this the default behaviour on the web already? When an <img> is used on the web, the browser fetches the image, calculates the width and height and maintains the aspect ratio of the image automatically. See this JSBin, try resizing the output panel and note that when the width of the image is changing, the height is also changing. Hence the aspect ratio of the image is automatically maintained by the browser.

Now in React Native, we do not load images and calculate dimensions, we expect the dimensions to be passed before-hand as part of style. But then we lose the capability of changing height of image in accordance with the width of the image.

This PR, helps us give the original intrinsicHeight and intrinsicWidth of the image (which a browser finds out after actually loading the image, while we provide beforehand) - and based on this, using the measure method of css-layout, we are able to maintain the aspect ratio of the image when it's width changes.

In much this way, the src attribute on the web, maps to the source attribute in React Native. Only that, in addition to the uri, we also need to give the width and height of the image - completely describing the remote resource.

<Image source={{
  uri: 'https://upload.wikimedia.org/wikipedia/commons/thumb/5/57/React.js_logo.svg/600px-React.js_logo.svg.png',
  width: 600,
  height: 600
}} />
@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Jun 18, 2015

Contributor

Hey @paramaggarwal, great work!

I'm wondering if we could use the same logic for both static and network images? Or am I missing something?

For more context about the image work we are doing internally. The idea is to deprecate require('image!...') usage in favor of relative require styles (require('./logo.png')). There are lots of benefits to this system: no need to manually add images in Xcode, no need to recompile, images live alongside the code, etc. You can follow it here #1043

Implementation wise we will be serving the images over network in development mode and will bundle them with the app when submitting to AppStore, so the same image will be rendered using NetworkImage and StaticImage depending on environment. That means they have to behave the same way to avoid surprises.

As part of that, we made packager add width and height to image metadata, so the developer won't need to duplicate information that the system can figure out. The only problem with this approach is when the developer wants to use something like flex to size the image - width and height attributes we set get priority. One solution could be flattening style and checking if flex or alignSelf are set, and only if not - applying known width and height.

Contributor

frantic commented Jun 18, 2015

Hey @paramaggarwal, great work!

I'm wondering if we could use the same logic for both static and network images? Or am I missing something?

For more context about the image work we are doing internally. The idea is to deprecate require('image!...') usage in favor of relative require styles (require('./logo.png')). There are lots of benefits to this system: no need to manually add images in Xcode, no need to recompile, images live alongside the code, etc. You can follow it here #1043

Implementation wise we will be serving the images over network in development mode and will bundle them with the app when submitting to AppStore, so the same image will be rendered using NetworkImage and StaticImage depending on environment. That means they have to behave the same way to avoid surprises.

As part of that, we made packager add width and height to image metadata, so the developer won't need to duplicate information that the system can figure out. The only problem with this approach is when the developer wants to use something like flex to size the image - width and height attributes we set get priority. One solution could be flattening style and checking if flex or alignSelf are set, and only if not - applying known width and height.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 19, 2015

Collaborator

I'm wondering if we could use the same logic for both static and network images?

Yes, this should absolutely happen.

  • For truly remote images (http://example.com/logo.png) the programmer has to specify {width, height, uri} in the image source
  • For images that are locally available but are served remotely by the packager (./logo.png), the packager should read the intrinsic size of the image and replace require('./logo.png') with {width, height, uri}
  • For images that are locally available and are statically bundled, the packager should read the intrinsic size of the image and replace require('./logo.png') with {width, height, uri, isStatic}

Then, both the native implementations for static and remote images should take the provided intrinsic dimensions into account when computing layout.

The only problem with this approach is when the developer wants to use something like flex to size the image - width and height attributes we set get priority.

The width and height properties of the image source should not get priority. They are intrinsic dimensions, not the rendered dimensions.

Just like the web, they should be the defaults that the layout engine uses as a last resort. For example, if the programmer uses flex and we know the rendered width of the image should be 375px wide but don't know its rendered height, then the layout engine should calculate rendered_height = rendered_width * intrinsic_height / intrinsic_width. If the programmer specifies style={{width: 200}} but leaves off the height, then the layout engine should perform a similar calculation using the intrinsic aspect ratio to compute the rendered height. If the programmers doesn't specify any rendered dimensions (no flex, no width nor height in the style), then the image falls back to its intrinsic dimensions.

Collaborator

ide commented Jun 19, 2015

I'm wondering if we could use the same logic for both static and network images?

Yes, this should absolutely happen.

  • For truly remote images (http://example.com/logo.png) the programmer has to specify {width, height, uri} in the image source
  • For images that are locally available but are served remotely by the packager (./logo.png), the packager should read the intrinsic size of the image and replace require('./logo.png') with {width, height, uri}
  • For images that are locally available and are statically bundled, the packager should read the intrinsic size of the image and replace require('./logo.png') with {width, height, uri, isStatic}

Then, both the native implementations for static and remote images should take the provided intrinsic dimensions into account when computing layout.

The only problem with this approach is when the developer wants to use something like flex to size the image - width and height attributes we set get priority.

The width and height properties of the image source should not get priority. They are intrinsic dimensions, not the rendered dimensions.

Just like the web, they should be the defaults that the layout engine uses as a last resort. For example, if the programmer uses flex and we know the rendered width of the image should be 375px wide but don't know its rendered height, then the layout engine should calculate rendered_height = rendered_width * intrinsic_height / intrinsic_width. If the programmer specifies style={{width: 200}} but leaves off the height, then the layout engine should perform a similar calculation using the intrinsic aspect ratio to compute the rendered height. If the programmers doesn't specify any rendered dimensions (no flex, no width nor height in the style), then the image falls back to its intrinsic dimensions.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 19, 2015

Contributor

Thanks @frantic, the behaviour of static and remote images is now exactly the same after my latest changes (which I have squashed into a single commit). I've published an example to https://github.com/paramaggarwal/flex-image-demo to demonstrate this.

Thanks @ide, the behaviour you describe makes perfect sense, please try out the example I have published. Here is the index.ios.js file of the demo.

Contributor

paramaggarwal commented Jun 19, 2015

Thanks @frantic, the behaviour of static and remote images is now exactly the same after my latest changes (which I have squashed into a single commit). I've published an example to https://github.com/paramaggarwal/flex-image-demo to demonstrate this.

Thanks @ide, the behaviour you describe makes perfect sense, please try out the example I have published. Here is the index.ios.js file of the demo.

@paramaggarwal paramaggarwal changed the title from [Image] Maintaining aspect ratio on network images. to [Image] Maintaining aspect ratio on images with flex layout. Jun 19, 2015

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 19, 2015

Contributor

I am trying the test cases described by @ide above.

If the programmer uses flex and we know the rendered width of the image should be 375px wide but don't know its rendered height, then the layout engine should calculate rendered_height = rendered_width * intrinsic_height / intrinsic_width.

This works, but without having to explicitly use flex. More details below.

If the programmer specifies style={{width: 200}} but leaves off the height, then the layout engine should perform a similar calculation using the intrinsic aspect ratio to compute the rendered height.

This works as expected.

If the programmers doesn't specify any rendered dimensions (no flex, no width nor height in the style), then the image falls back to its intrinsic dimensions.

This does not work as expected. When no style dimensions are specified, it is picking width as the width of the parent.

I attempted to fix this by trying these modifications in my code:

  • If the width of the image is smaller than the width of parent, then return the width of the image as is, with the height calculated in proportion.

But this still didn't work because css-layout uses only the height from the returned dimensions and not the width. It continues to use the width of the parent itself. The relevant snippet of code is here.

  • If there was a way to check if flex is being used on the node, then return intrinsic dimensions as is.
    Doing this is hard from outside of css-layout, as detecting whether flex is being used can only be determined from inside the css-layout code. This is because flex could be cascading down the children.

@vjeux please help with your thoughts here. Should this kind of layout be a method on css-layout instead? For example right now, we specify a measure function for css-layout, but instead if we could pass the intrinsic dimensions of the node to css-layout itself? That way this could be applied to any node, not just images. Please do share thoughts.

Contributor

paramaggarwal commented Jun 19, 2015

I am trying the test cases described by @ide above.

If the programmer uses flex and we know the rendered width of the image should be 375px wide but don't know its rendered height, then the layout engine should calculate rendered_height = rendered_width * intrinsic_height / intrinsic_width.

This works, but without having to explicitly use flex. More details below.

If the programmer specifies style={{width: 200}} but leaves off the height, then the layout engine should perform a similar calculation using the intrinsic aspect ratio to compute the rendered height.

This works as expected.

If the programmers doesn't specify any rendered dimensions (no flex, no width nor height in the style), then the image falls back to its intrinsic dimensions.

This does not work as expected. When no style dimensions are specified, it is picking width as the width of the parent.

I attempted to fix this by trying these modifications in my code:

  • If the width of the image is smaller than the width of parent, then return the width of the image as is, with the height calculated in proportion.

But this still didn't work because css-layout uses only the height from the returned dimensions and not the width. It continues to use the width of the parent itself. The relevant snippet of code is here.

  • If there was a way to check if flex is being used on the node, then return intrinsic dimensions as is.
    Doing this is hard from outside of css-layout, as detecting whether flex is being used can only be determined from inside the css-layout code. This is because flex could be cascading down the children.

@vjeux please help with your thoughts here. Should this kind of layout be a method on css-layout instead? For example right now, we specify a measure function for css-layout, but instead if we could pass the intrinsic dimensions of the node to css-layout itself? That way this could be applied to any node, not just images. Please do share thoughts.

@frantic

This comment has been minimized.

Show comment
Hide comment
@frantic

frantic Jun 19, 2015

Contributor

@ide great writeup! @paramaggarwal I'm happy with your approach, will let @vjeux stamp this.

Contributor

frantic commented Jun 19, 2015

@ide great writeup! @paramaggarwal I'm happy with your approach, will let @vjeux stamp this.

@frantic frantic assigned vjeux and unassigned frantic Jun 19, 2015

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 21, 2015

Contributor

The measure method capability exposed by css-layout was originally intended only for text. This allows any node to define a function that is passed a width, and it is able to return a width and height at runtime. Hence RCTText nodes use this, and don't need explicit width height styles. Text simply takes up the amount of height that it needs. This is also how text layout works in the browser.

Now to use this capability for images, and behave like how browsers treat images, we hit into the two following limitations of the measure method.

  1. It uses the width of the parent node, irrespective of flex being active or not.
  2. It uses the width of the parent node, irrespective of width returned from the measure method.

That is, it is very adamant about the width being equal to the parent node, unless explicitly defined in style. It does not even expect an alignSelf: 'stretch' on the node or alignItems: 'stretch' on the parent. This makes perfect sense for the original use case of a text node, but not for an image node.

To be able to do layout for image nodes, either we modify the above two behaviours. But I realised that the math we do for image layout is pretty simple, and does not depend on platform specific methods (how in measure we need to use platform specific APIs for text sizing.)

Hence, instead of defining a measure method, if one could set intrinsicDimensions of a css-node, then css-layout could automatically do the math, which would be as follows (from @ide's comment):

  1. If flex active, use parent width as child width, and calculate child height in proportion for flexDirection: 'column', and other way for flexDirection: 'row'.
  2. If width dimension specified in style, then calculate height using intrinsicDimensions.
  3. If no style, use intrinsicDimensions as is.

This was similar to the measure opt-in, this would give an intrinsicDimensions opt-in on css-layout specific to images, but also for nodes that wish to support custom aspectRatios. Also this is completely opt-in behaviour, would not break existing logic because nothing would change is intrinsicDimensions were not specified. Also, as none of this comes from style itself, this would not be against the CSS spec.

Please do let me know if this sounds correct - this would mean that we'll need to port these changes to css-layout, which I'm not very familiar with!

Contributor

paramaggarwal commented Jun 21, 2015

The measure method capability exposed by css-layout was originally intended only for text. This allows any node to define a function that is passed a width, and it is able to return a width and height at runtime. Hence RCTText nodes use this, and don't need explicit width height styles. Text simply takes up the amount of height that it needs. This is also how text layout works in the browser.

Now to use this capability for images, and behave like how browsers treat images, we hit into the two following limitations of the measure method.

  1. It uses the width of the parent node, irrespective of flex being active or not.
  2. It uses the width of the parent node, irrespective of width returned from the measure method.

That is, it is very adamant about the width being equal to the parent node, unless explicitly defined in style. It does not even expect an alignSelf: 'stretch' on the node or alignItems: 'stretch' on the parent. This makes perfect sense for the original use case of a text node, but not for an image node.

To be able to do layout for image nodes, either we modify the above two behaviours. But I realised that the math we do for image layout is pretty simple, and does not depend on platform specific methods (how in measure we need to use platform specific APIs for text sizing.)

Hence, instead of defining a measure method, if one could set intrinsicDimensions of a css-node, then css-layout could automatically do the math, which would be as follows (from @ide's comment):

  1. If flex active, use parent width as child width, and calculate child height in proportion for flexDirection: 'column', and other way for flexDirection: 'row'.
  2. If width dimension specified in style, then calculate height using intrinsicDimensions.
  3. If no style, use intrinsicDimensions as is.

This was similar to the measure opt-in, this would give an intrinsicDimensions opt-in on css-layout specific to images, but also for nodes that wish to support custom aspectRatios. Also this is completely opt-in behaviour, would not break existing logic because nothing would change is intrinsicDimensions were not specified. Also, as none of this comes from style itself, this would not be against the CSS spec.

Please do let me know if this sounds correct - this would mean that we'll need to port these changes to css-layout, which I'm not very familiar with!

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 24, 2015

Contributor

I just successfully tried out a standalone <AspectRatio /> component. It looks like this, and does not require any changes to the RCTImage module. It is also completely opt-in and for generic use on any component whatsoever. The way it works is that it takes the input dimensions and uses them to size itself into the constraints of the parent view. Hence always maintaining the specified aspect ratio.

<AspectRatio dimensions={{width: 980, height: 480}}>
  <Image style={{flex: 1}} source={{uri: "http://example.com/image.jpg"}} />
</AspectRatio>

Unfortunately, I cannot publish as it needs this change from css-layout imported into React Native: facebook/yoga#84.

Contributor

paramaggarwal commented Jun 24, 2015

I just successfully tried out a standalone <AspectRatio /> component. It looks like this, and does not require any changes to the RCTImage module. It is also completely opt-in and for generic use on any component whatsoever. The way it works is that it takes the input dimensions and uses them to size itself into the constraints of the parent view. Hence always maintaining the specified aspect ratio.

<AspectRatio dimensions={{width: 980, height: 480}}>
  <Image style={{flex: 1}} source={{uri: "http://example.com/image.jpg"}} />
</AspectRatio>

Unfortunately, I cannot publish as it needs this change from css-layout imported into React Native: facebook/yoga#84.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 28, 2015

Contributor

@paramaggarwal: omg this is awesome! Having a standalone component is the best possible outcome, I can't wait to see your implementation

Can you submit a pull request that just add this css-layout change to react-native repo? I'll happily stamp and import it.

Contributor

vjeux commented Jun 28, 2015

@paramaggarwal: omg this is awesome! Having a standalone component is the best possible outcome, I can't wait to see your implementation

Can you submit a pull request that just add this css-layout change to react-native repo? I'll happily stamp and import it.

@paramaggarwal paramaggarwal changed the title from [Image] Maintaining aspect ratio on images with flex layout. to [View] Keep view's aspect ratio based on new prop Jun 29, 2015

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 29, 2015

Contributor

Thanks @vjeux. I have submitted #1776, updated this PR and uploaded a demo here: https://github.com/paramaggarwal/aspect-ratio-demo.

It looks like this for an Image:

<Image
  aspectRatioBox={{
    width: 612,
    height: 456,
  }}
  source={{
    uri: "http://example.com/image.jpg",
  }}
/>

and a View that should always be a square, and then flex layout everything inside it:

<View aspectRatioBox={{width: 1, height: 1}}>
  <View style={{flexDirection: 'row', flex: 1}}>
    <View style={{flex: 1, backgroundColor: 'red'}} />
    <View style={{flex: 2, backgroundColor: 'green'}} />
  </View>
  <View style={{flexDirection: 'row', flex: 1}}>
    <View style={{flex: 3, backgroundColor: 'blue'}} />
    <View style={{flex: 2, backgroundColor: 'yellow'}} />
  </View>
</View>

I thought about it, and decided to make this an opt-in aspectRatioBox prop on any generic node because it seemed like a valid use-case to actually be able to use this anywhere at all. Please do share your thoughts ion the naming of the aspectRatioBox prop and the location of this code.

Contributor

paramaggarwal commented Jun 29, 2015

Thanks @vjeux. I have submitted #1776, updated this PR and uploaded a demo here: https://github.com/paramaggarwal/aspect-ratio-demo.

It looks like this for an Image:

<Image
  aspectRatioBox={{
    width: 612,
    height: 456,
  }}
  source={{
    uri: "http://example.com/image.jpg",
  }}
/>

and a View that should always be a square, and then flex layout everything inside it:

<View aspectRatioBox={{width: 1, height: 1}}>
  <View style={{flexDirection: 'row', flex: 1}}>
    <View style={{flex: 1, backgroundColor: 'red'}} />
    <View style={{flex: 2, backgroundColor: 'green'}} />
  </View>
  <View style={{flexDirection: 'row', flex: 1}}>
    <View style={{flex: 3, backgroundColor: 'blue'}} />
    <View style={{flex: 2, backgroundColor: 'yellow'}} />
  </View>
</View>

I thought about it, and decided to make this an opt-in aspectRatioBox prop on any generic node because it seemed like a valid use-case to actually be able to use this anywhere at all. Please do share your thoughts ion the naming of the aspectRatioBox prop and the location of this code.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jun 29, 2015

Contributor

The fact that the argument is a vector is going to be pretty hard to convey. aspectRatio is scalar and giving it a vector is tricky. Also, one very common use case is to want a square aspect ratio which is awkward with your API.

One thing we can do is to name it aspectRatio and be able to take either a number or a vector. This way both use cases are properly covered and you can use the one that's most appropriate.

Contributor

vjeux commented Jun 29, 2015

The fact that the argument is a vector is going to be pretty hard to convey. aspectRatio is scalar and giving it a vector is tricky. Also, one very common use case is to want a square aspect ratio which is awkward with your API.

One thing we can do is to name it aspectRatio and be able to take either a number or a vector. This way both use cases are properly covered and you can use the one that's most appropriate.

Show outdated Hide outdated Libraries/Components/View/View.js
* to wrap images and other components, where you would like the aspect
* ratio to be maintained.
*/
aspectRatioBox: PropTypes.shape({

This comment has been minimized.

@vjeux

vjeux Jun 29, 2015

Contributor

you also need to add this to <Image>

@vjeux

vjeux Jun 29, 2015

Contributor

you also need to add this to <Image>

Show outdated Hide outdated React/Views/RCTShadowView.m
- (void)setAspectRatioBox:(CGSize)aspectRatioBox {
_aspectRatioBox = aspectRatioBox;
if (aspectRatioBox.width && aspectRatioBox.height) {
self.cssNode->measure = RCTAspectRatioMeasure;

This comment has been minimized.

@vjeux

vjeux Jun 29, 2015

Contributor

nit: indent 2 spaces

@vjeux

vjeux Jun 29, 2015

Contributor

nit: indent 2 spaces

Show outdated Hide outdated React/Views/RCTShadowView.m
*/
static css_dim_t RCTAspectRatioMeasure(void *context, float width)
{
RCTShadowView *shadowView = (__bridge RCTShadowView *)context;

This comment has been minimized.

@vjeux

vjeux Jun 29, 2015

Contributor

nit: 2 spaces

@vjeux

vjeux Jun 29, 2015

Contributor

nit: 2 spaces

Show outdated Hide outdated React/Views/RCTShadowView.m
static css_dim_t RCTAspectRatioMeasure(void *context, float width)
{
RCTShadowView *shadowView = (__bridge RCTShadowView *)context;
if (isnan(width)) width = shadowView.aspectRatioBox.width;

This comment has been minimized.

@vjeux

vjeux Jun 29, 2015

Contributor

nit: please always put braces and the statement on the next line

@vjeux

vjeux Jun 29, 2015

Contributor

nit: please always put braces and the statement on the next line

Show outdated Hide outdated React/Views/RCTShadowView.m
}
css_dim_t result;
result.dimensions[CSS_WIDTH] = ceilf(width);

This comment has been minimized.

@vjeux

vjeux Jun 29, 2015

Contributor

Please don't round here. We only want to round exactly once when we apply it to the view, otherwise we're losing precision all along the way.

And, if we were to round here, we would need to ask many questions like

  • why ceil instead of floor or round?
  • the pixel density needs to be taken into account. 1 unit is equivalent to 2 physical pixels on iphone 5 for example. If you were to do this, you would lose half your precision
@vjeux

vjeux Jun 29, 2015

Contributor

Please don't round here. We only want to round exactly once when we apply it to the view, otherwise we're losing precision all along the way.

And, if we were to round here, we would need to ask many questions like

  • why ceil instead of floor or round?
  • the pixel density needs to be taken into account. 1 unit is equivalent to 2 physical pixels on iphone 5 for example. If you were to do this, you would lose half your precision
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 29, 2015

Collaborator

Regarding the prop name I'd vouch either for intrinsicSize={size} or aspectRatio={number}. I can see scenarios in which the intrinsic size is useful (images, also 3d views if RN ever wants to do that) and scenarios in which a component author only wants to provide an aspect ratio (vector graphics).

I think the answer is that we might want both (and it is a warning if they don't match). http://dev.w3.org/csswg/css-images-3/#intrinsic-dimensions says

Raster images are an example of an object with all three intrinsic dimensions. SVG images designed to scale might have only an intrinsic aspect ratio; SVG images can also be created with only an intrinsic width or height.

name it aspectRatio and be able to take either a number or a vector

this seems quite reasonable to me and very nicely prevents you from specifying intrinsic dimensions and an aspect ratio that don't agree with each other.

Collaborator

ide commented Jun 29, 2015

Regarding the prop name I'd vouch either for intrinsicSize={size} or aspectRatio={number}. I can see scenarios in which the intrinsic size is useful (images, also 3d views if RN ever wants to do that) and scenarios in which a component author only wants to provide an aspect ratio (vector graphics).

I think the answer is that we might want both (and it is a warning if they don't match). http://dev.w3.org/csswg/css-images-3/#intrinsic-dimensions says

Raster images are an example of an object with all three intrinsic dimensions. SVG images designed to scale might have only an intrinsic aspect ratio; SVG images can also be created with only an intrinsic width or height.

name it aspectRatio and be able to take either a number or a vector

this seems quite reasonable to me and very nicely prevents you from specifying intrinsic dimensions and an aspect ratio that don't agree with each other.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jun 30, 2015

Contributor

@vjeux thanks for the in-depth review - I have incorporated all your feedback.
@ide @vjeux aspectRatio with type-checking sounds perfect, I've implemented this:

aspectRatio: React.PropTypes.oneOfType([
  React.PropTypes.number,
  React.PropTypes.shape({
      width: React.PropTypes.number,
      height: React.PropTypes.number,
    }),
]),

I've also updated the demo here: https://github.com/paramaggarwal/aspect-ratio-demo.

Contributor

paramaggarwal commented Jun 30, 2015

@vjeux thanks for the in-depth review - I have incorporated all your feedback.
@ide @vjeux aspectRatio with type-checking sounds perfect, I've implemented this:

aspectRatio: React.PropTypes.oneOfType([
  React.PropTypes.number,
  React.PropTypes.shape({
      width: React.PropTypes.number,
      height: React.PropTypes.number,
    }),
]),

I've also updated the demo here: https://github.com/paramaggarwal/aspect-ratio-demo.

@ryangomba

This comment has been minimized.

Show comment
Hide comment
@ryangomba

ryangomba Jul 2, 2015

Contributor

I like this a lot and have found it useful myself. However, you're missing RCTShadowView's setAspectRatio implementation in this diff :)

Contributor

ryangomba commented Jul 2, 2015

I like this a lot and have found it useful myself. However, you're missing RCTShadowView's setAspectRatio implementation in this diff :)

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jul 2, 2015

Contributor

@ryangomba thank you so much! Missed it while copying over changes.

Contributor

paramaggarwal commented Jul 2, 2015

@ryangomba thank you so much! Missed it while copying over changes.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jul 9, 2015

Contributor

@vjeux Please review!

Contributor

paramaggarwal commented Jul 9, 2015

@vjeux Please review!

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 11, 2015

Collaborator

@paramaggarwal - I think he is on vacation still but I think he should be available next week :)

Collaborator

brentvatne commented Jul 11, 2015

@paramaggarwal - I think he is on vacation still but I think he should be available next week :)

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jul 22, 2015

Collaborator

ping @vjeux 😍

Collaborator

brentvatne commented Jul 22, 2015

ping @vjeux 😍

@dubert

This comment has been minimized.

Show comment
Hide comment
@dubert

dubert Jul 26, 2015

Contributor

👍

What happens if you don't know the aspect ratio from a network image? Is there a way to retrieve the source Height and Width to get the aspect ratio?

Contributor

dubert commented Jul 26, 2015

👍

What happens if you don't know the aspect ratio from a network image? Is there a way to retrieve the source Height and Width to get the aspect ratio?

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jul 27, 2015

Contributor

@dubert There is no way at the moment. Might be useful to have it on let's say an onLoad method of an <Image> tag.

Contributor

paramaggarwal commented Jul 27, 2015

@dubert There is no way at the moment. Might be useful to have it on let's say an onLoad method of an <Image> tag.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jul 27, 2015

Contributor

ping @vjeux @ide @sahrens. Please share your thoughts!

Contributor

paramaggarwal commented Jul 27, 2015

ping @vjeux @ide @sahrens. Please share your thoughts!

@@ -275,4 +275,14 @@ - (RCTViewEventHandler)eventHandlerWithName:(NSString *)eventName json:(id)json
RCT_REMAP_SHADOW_PROPERTY(onLayout, hasOnLayout, BOOL)
RCT_CUSTOM_SHADOW_PROPERTY(aspectRatio, CGSize, RCTShadowView) {

This comment has been minimized.

@ide

ide Jul 28, 2015

Collaborator

I believe you should handle the case where json is nil with defaultView.aspectRatio.

@ide

ide Jul 28, 2015

Collaborator

I believe you should handle the case where json is nil with defaultView.aspectRatio.

This comment has been minimized.

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

fixed

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

fixed

Show outdated Hide outdated React/Views/RCTShadowView.m
/*
* Sets the measure function on the cssNode, when the
* aspectRatio rectangle is set. This helps calculate
* the height of the node based on width of parent and

This comment has been minimized.

@ide

ide Jul 28, 2015

Collaborator

"...width of [the] parent..."

also can you adjust the line width of the comment to be 80 chars?

@ide

ide Jul 28, 2015

Collaborator

"...width of [the] parent..."

also can you adjust the line width of the comment to be 80 chars?

This comment has been minimized.

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

fixed

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

fixed

result.dimensions[CSS_WIDTH] = width;
result.dimensions[CSS_HEIGHT] = computedHeight;
return result;
}

This comment has been minimized.

@ide

ide Jul 28, 2015

Collaborator

Ideally, the aspect ratio would support views that have a known height (either hardcoded height from the stylesheet, or alignSelf: stretch in a horizontal parent) and unknown width. Any way to support this?

@ide

ide Jul 28, 2015

Collaborator

Ideally, the aspect ratio would support views that have a known height (either hardcoded height from the stylesheet, or alignSelf: stretch in a horizontal parent) and unknown width. Any way to support this?

This comment has been minimized.

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

Ah, good catch. Unfortunately this needs work on the css-layout side. Currently because this was intended for text layout, it only deals with height based on width scenario...

@paramaggarwal

paramaggarwal Jul 28, 2015

Contributor

Ah, good catch. Unfortunately this needs work on the css-layout side. Currently because this was intended for text layout, it only deals with height based on width scenario...

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Aug 6, 2015

Contributor

Folks, please review! Also, if there is anything that should be done differently here, please share!

Contributor

paramaggarwal commented Aug 6, 2015

Folks, please review! Also, if there is anything that should be done differently here, please share!

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Aug 17, 2015

Contributor

Please review/share feedback!

Contributor

paramaggarwal commented Aug 17, 2015

Please review/share feedback!

@hayeah

This comment has been minimized.

Show comment
Hide comment
@hayeah

hayeah Aug 18, 2015

Contributor

I've been using aspectRatio from @paramaggarwal's branch, and have not run into any problem. Absolutely essential! 👍

Contributor

hayeah commented Aug 18, 2015

I've been using aspectRatio from @paramaggarwal's branch, and have not run into any problem. Absolutely essential! 👍

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Aug 21, 2015

Contributor

Folks, please review!

Contributor

paramaggarwal commented Aug 21, 2015

Folks, please review!

@hayeah

This comment has been minimized.

Show comment
Hide comment
@hayeah

hayeah Aug 22, 2015

Contributor

ping @vjeux @ide @sahrens. Could aspectRatio make it into 0.10?

Contributor

hayeah commented Aug 22, 2015

ping @vjeux @ide @sahrens. Could aspectRatio make it into 0.10?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 22, 2015

Collaborator

It won't go into 0.10 because we've cut the RC and this isn't an urgent bugfix. I don't think it will go into 0.11 either because that release will focus on stability, but it could make it into 0.12-rc.

For me to feel good about this commit I'd want aspectRatio to work in both dimensions. E.g. if you have an image with aspect ratio 4:3 and give it height: 300, it should be rendered at 400x300. Likewise it should be rendered at 400x300 if you specify width: 400 too.

Collaborator

ide commented Aug 22, 2015

It won't go into 0.10 because we've cut the RC and this isn't an urgent bugfix. I don't think it will go into 0.11 either because that release will focus on stability, but it could make it into 0.12-rc.

For me to feel good about this commit I'd want aspectRatio to work in both dimensions. E.g. if you have an image with aspect ratio 4:3 and give it height: 300, it should be rendered at 400x300. Likewise it should be rendered at 400x300 if you specify width: 400 too.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Aug 22, 2015

Contributor

@ide Good point! That needs a change in css-layout - will leave this open till we get around to supporting it there.

Contributor

paramaggarwal commented Aug 22, 2015

@ide Good point! That needs a change in css-layout - will leave this open till we get around to supporting it there.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Aug 28, 2015

Contributor

Till then, could someone please pull #1776? It was imported but never showed up on master. With that we can use this component separately as a library. It's merged upstream also: facebook/yoga#84.

Contributor

paramaggarwal commented Aug 28, 2015

Till then, could someone please pull #1776? It was imported but never showed up on master. With that we can use this component separately as a library. It's merged upstream also: facebook/yoga#84.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Sep 6, 2015

Contributor

Anybody would like to help out on facebook/yoga#118? The idea is to make measure more generic and be more like sizeThatFits from iOS. (ref: sizeThatFits)

Contributor

paramaggarwal commented Sep 6, 2015

Anybody would like to help out on facebook/yoga#118? The idea is to make measure more generic and be more like sizeThatFits from iOS. (ref: sizeThatFits)

@hayeah

This comment has been minimized.

Show comment
Hide comment
@hayeah

hayeah Sep 11, 2015

Contributor

@paramaggarwal is there another issue I should watch? Would really like to see this happen...

Contributor

hayeah commented Sep 11, 2015

@paramaggarwal is there another issue I should watch? Would really like to see this happen...

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Sep 11, 2015

Contributor

@hayeah Long story short, this PR depended on measure from css-layout to override layout when needed - for example aspect ratio. But measure was originally meant for only text layout, which means it only works to calculate height, given a width. I tried my hand at adding support to measure to also work the other way - calculate the width, given a height.

Basically the goal was to make measure a more generic way to override layout whenever a component needs it. Just like sizeThatFits: from iOS UIView. But I tried hard (facebook/yoga#118) and failed to make css-layout work that way. I don't have much knowledge of how css-layout works.

Currently I am making do with having a fork of RN with #1776 merged in and then just having a small custom component called RigidBox to maintain aspect ratio of my images.

Contributor

paramaggarwal commented Sep 11, 2015

@hayeah Long story short, this PR depended on measure from css-layout to override layout when needed - for example aspect ratio. But measure was originally meant for only text layout, which means it only works to calculate height, given a width. I tried my hand at adding support to measure to also work the other way - calculate the width, given a height.

Basically the goal was to make measure a more generic way to override layout whenever a component needs it. Just like sizeThatFits: from iOS UIView. But I tried hard (facebook/yoga#118) and failed to make css-layout work that way. I don't have much knowledge of how css-layout works.

Currently I am making do with having a fork of RN with #1776 merged in and then just having a small custom component called RigidBox to maintain aspect ratio of my images.

@bakso

This comment has been minimized.

Show comment
Hide comment
@bakso

bakso Sep 16, 2015

Very well done! But where is the doc?

bakso commented Sep 16, 2015

Very well done! But where is the doc?

@wootwoot1234

This comment has been minimized.

Show comment
Hide comment
@wootwoot1234

wootwoot1234 Nov 1, 2015

Any update on this? I could really use this in my project right now. :)

Any update on this? I could really use this in my project right now. :)

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Nov 2, 2015

Contributor

@iostalk @wootwoot1234 The changes in this PR are able to support aspect ratio only in one dimension - for it to make it into core, it needs to support both vertical and horizontal dimensions. Unfortunately that needs serious rework on the css-layout repo.

Contributor

paramaggarwal commented Nov 2, 2015

@iostalk @wootwoot1234 The changes in this PR are able to support aspect ratio only in one dimension - for it to make it into core, it needs to support both vertical and horizontal dimensions. Unfortunately that needs serious rework on the css-layout repo.

@alfrednerstu

This comment has been minimized.

Show comment
Hide comment
@alfrednerstu

alfrednerstu Nov 24, 2015

Also needing this badly, what are the latest developments? Is there a way I can get this to work @paramaggarwal ?

Also needing this badly, what are the latest developments? Is there a way I can get this to work @paramaggarwal ?

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Nov 25, 2015

Contributor

@alfrednerstu I should probably publish what I have as an OSS component. Haven't gotten around to doing it...

Contributor

paramaggarwal commented Nov 25, 2015

@alfrednerstu I should probably publish what I have as an OSS component. Haven't gotten around to doing it...

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Jan 14, 2016

Contributor

Hey folks, here is the generic component I have been using to make an arbitrary container who's height is always in a particular ratio to its width. This width is equal to the width of the parent. Great for use with images where you want them to stick to a particular aspect ratio.

Here is the code with the usage as a comment: https://gist.github.com/paramaggarwal/b9baa00fd7bee9a2948f

Feel free to use the code anywhere and also make it into an OSS component.

Contributor

paramaggarwal commented Jan 14, 2016

Hey folks, here is the generic component I have been using to make an arbitrary container who's height is always in a particular ratio to its width. This width is equal to the width of the parent. Great for use with images where you want them to stick to a particular aspect ratio.

Here is the code with the usage as a comment: https://gist.github.com/paramaggarwal/b9baa00fd7bee9a2948f

Feel free to use the code anywhere and also make it into an OSS component.

@paramaggarwal

This comment has been minimized.

Show comment
Hide comment
@paramaggarwal

paramaggarwal Feb 23, 2016

Contributor

The changes needed in css-layout are now in: facebook/yoga#154 - but I'll need to integrate the Android version also. @obipawan has implemented it internally. Will have to also add an example in UIExplorer.

Should I make the change only for Image component, or generically available on View?

Contributor

paramaggarwal commented Feb 23, 2016

The changes needed in css-layout are now in: facebook/yoga#154 - but I'll need to integrate the Android version also. @obipawan has implemented it internally. Will have to also add an example in UIExplorer.

Should I make the change only for Image component, or generically available on View?

@polarathene

This comment has been minimized.

Show comment
Hide comment
@polarathene

polarathene Mar 13, 2016

@paramaggarwal It would be nice as generic. I have a use case with react-native-video component where I'm wanting to use flex:1 for the width but maintain aspect ratio without having to provide a fixed height.

I'm curious if this feature might help towards implementing flex-grow/shrink features?

@paramaggarwal It would be nice as generic. I have a use case with react-native-video component where I'm wanting to use flex:1 for the width but maintain aspect ratio without having to provide a fixed height.

I'm curious if this feature might help towards implementing flex-grow/shrink features?

@mschipperheyn

This comment has been minimized.

Show comment
Hide comment
@mschipperheyn

mschipperheyn Mar 22, 2016

This feels a bit like css's: background-size:cover. Ideally, you would be able to specify how to position the image in the defined window. Normally, a centered approach would be best

This feels a bit like css's: background-size:cover. Ideally, you would be able to specify how to position the image in the defined window. Normally, a centered approach would be best

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Mar 22, 2016

@paramaggarwal updated the pull request.

@paramaggarwal updated the pull request.

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