Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add support for local images #16

Open
brh55 opened this issue May 3, 2017 · 24 comments
Open

Re-add support for local images #16

brh55 opened this issue May 3, 2017 · 24 comments
Assignees

Comments

@brh55
Copy link
Owner

brh55 commented May 3, 2017

Looks like I may have missed it during development of v0.1.0.

Reproduction Instructions:

  • Set Props.bricks = [LocalImages (require('../asset/etc')]
  • Images won't load

Validate:

  • When passed in a local image, it should appear in the columns
@dazziola
Copy link

dazziola commented May 4, 2017

Does this mean the example is void? I had a look through your code and it appears that you are indeed using uri in the source prop of the Image component:

    <Image
        key={image.uri}
        source={{ uri: image.uri }}
        resizeMethod='auto'
        style={{ width: image.width, height: image.height, marginTop: gutter }} />

Just trying to get it running myself with remote images!

@brh55 brh55 changed the title Remote images Local images May 4, 2017
@brh55 brh55 changed the title Local images Re-add support for local images May 4, 2017
@brh55
Copy link
Owner Author

brh55 commented May 4, 2017

@dazziola
Ahh my apologies, I meant to put "local" images -- just corrected it. But yes, remote images will work. Let me know if you are having issues with getting it to work.

Thanks for catching that.

@dazziola
Copy link

dazziola commented May 4, 2017

Hmmm, I'm having a weird non-descript problem. I'm using the data block of images as per your example with two columns, however they're not rendering in the simulator or on a physical device.

@brh55
Copy link
Owner Author

brh55 commented May 4, 2017

@dazziola What version of RN and React are you running?

@dazziola
Copy link

dazziola commented May 4, 2017

"react-native": "0.43.3",
"react": "16.0.0-alpha.6",

@brh55
Copy link
Owner Author

brh55 commented May 4, 2017

@dazziola
Hmm I just ran a fresh install of the example, no issues currently. Are your images HTTPS or HTTP? If you are on IOS, by default IOS causes non-secure images to fail (facebook/react-native#8520).

Also does your debug log show up any information? react-native-masonry will inform you if an image has failed to load as a warning, so you should see that if it's an issue with loading the network image.

@dazziola
Copy link

dazziola commented May 4, 2017

I've cloned the example and you're right, it works perfectly. Hmmm, I'll mess around with my one and see if I can figure out what it is. Thanks for the prompt responses! If it's anything other than my own silly oversight, I'll be sure to post it here for future people!

@brh55
Copy link
Owner Author

brh55 commented May 4, 2017

@dazziola
Absolutely! Please let me know what the issue was, maybe we can clear up or improve the readme 👍 .

@brh55 brh55 self-assigned this May 26, 2017
@brh55
Copy link
Owner Author

brh55 commented May 26, 2017

Still currently trying to resolve this issue without the need to require a local image prop or something like that. Ideally I want the users to be able to set { uri: localFilePath }, which currently resolves to a number -- I believe is the id within the asset manager. If anyone knows, please lmk!

@fxfactorial
Copy link

@brh55 Well why are you assuming source={{ uri: image.uri }}, I wouldn't do that. Just let the user pass whatever, you're just a front for the Image API

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

@fxfactorial Would work, but then local images wouldn't be able to leverage other properties such as onPress, renderHeader, renderFooter.

@fxfactorial
Copy link

@brh55 How so?

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

@fxfactorial I'm assuming you are referring to users passing in react elements like
bricks = [<Image source={require('../asset1.jpg')} />, <Image source={uri: 'http://image.com/url1.jpg' } />]?

@fxfactorial
Copy link

Hmm, I wasn't but that seems like a fine solution as well.

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

Ahh what other way are you referring to?

@fxfactorial
Copy link

bricks = [{img: require('../asset1.jpg')}, {img: {uri: 'https:....'}}]

I mean, just a front for the source prop of Image.

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

OHH yeah that should work, I'm not sure why I was having issues before.

@brh55 brh55 added v0.5.0 and removed v0.1.0 labels Jun 21, 2017
@fxfactorial
Copy link

fxfactorial commented Jun 21, 2017

@brh55 actually now I'm thinking, should let end user provide their own Image component, or whatever view they give. Why assume masonry desired is image?

For example, what if I wanted to use this: https://github.com/oblador/react-native-image-progress

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

@fxfactorial Interesting yeah you could overload the Image component with any desired image component. Well without images, it's rather straightforward to achieve the sme effect: flex: row, justifyContent: space-between.

So RNM was initially to solve this issue using remote images, since it needs to fetch the image and pass on the proper size to the image component. But RNM has grown to fit others needs along the way haha.

@fxfactorial
Copy link

How do you mean to overload?

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

Ah let me rephrase that -- I think it would be neat if you could use a custom image component, and it's probably possible.

By having a property that could allow users to pass a component for RNM to use in the rendering process instead of the default image component -- assuming the third-party component had source as a property.

I'm sure there is another way of doing this that I perhaps am skipping over.

@fxfactorial
Copy link

right, definitely some room for generalization

@brh55
Copy link
Owner Author

brh55 commented Jun 21, 2017

Indeed, theoretically it should work.

@brh55 brh55 added this to TODO in v0.4.0 Aug 16, 2017
@brh55
Copy link
Owner Author

brh55 commented Sep 11, 2017

Per suggestions, there is still some underlying bundling that is occurring not allowing it to work as imagined technically. Going to leave this open, and welcomed for anyone to try and come up with a viable solution.

@brh55 brh55 removed this from TODO in v0.4.0 Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants