-
Notifications
You must be signed in to change notification settings - Fork 28
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
Replace Respective Image using Custom Component #20
Conversation
Hey, @jasimawan thank you for this great! Wanna ask about replacing the image, it seems that you're setting the whole state of images, but this will not re-rerender the child component DynamicCollage? or I'm missing something? I was thinking that you have 3 images and want to replace one, the whole state would change and the state of the other two would get lost? |
Hi, @harbolaez thank you for the kind response. No it will only change the image we want to replace, the other images in the list will remain the same. You can see in the example code of onEditButtonPress. I made a copy of my images list and replaced the image which i want to be replaced with new image fetched from Image Picker and after that I update the state of images with new updated list. Use my method in your app and you will understand how its updating the the list of images. Thanks. |
I have attached a gyzao on what I'm experiencing https://gyazo.com/928a6ddfbbfd0807da760421e42aca33 -- let me know what you think? This is the snippet of code i'm using
|
@harbolaez I checked your code and the url you are replacing your image with is not rendering correctly. Test it on some other images url. Secondly there is a problem with your images hook as its not updating the render function. Check your state as well. |
@jasimawan Thanks for the PR! Appreciate the excitement around this library :) Could you let me know your prettier configuration? then I can push an update on Master and prettify the codebase so we can see more clearly the changes you made. Also, this code here for updating is crazy 😆:
I wonder if we should add in a replace image API i.e. (I can push this one as a separate PR)
What do you think? |
Yes we can do that as well and I think your suggestion is better than my way of updating images. Glad you respond with a positive feedback and i will definitely enhance it using your suggestion and I will try to implement it this way. And if I feel to be stuck anywhere I will tell you so that you can help me in this regard. |
@jasimawan I have prettified the code-base, see: 20e1bb0 Would you be able to rebase so I can see your changes clearly in this PR? |
@lukebrandonfarrell The changes I made you can see them in |
Thanks @jasimawan ! Can see the changes much more clearly now :) Looks good, will test this on device this evening, then happy to merge and publish |
Thanks @lukebrandonfarrell for the kind response and to give me the opportunity to work on this Repo. Waiting for your next response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this! Just tested it and looks awesome :) A left a few comments, but after that I am happy to merge and push this as 3.3.0. Thanks again for the PR
src/DynamicCollage.js
Outdated
@@ -339,6 +343,10 @@ DynamicCollage.propTypes = { | |||
retainScaleOnSwap: PropTypes.bool, | |||
longPressDelay: PropTypes.number, | |||
longPressSensitivity: PropTypes.number, // 1 - 20 - How sensitive is the long press? | |||
onEditButtonPress: PropTypes.func, | |||
EditButtonComponent: PropTypes.func, | |||
editButtonPosition: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we change the prop type to an enum which accepts oneOf: "bottom-right", "bottom-left", "top-right" and "top-left" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can do that, I start working on that and will update the PR. @lukebrandonfarrell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added enum types in the library and if someone will not enter the right enum type, the app will not crash and the the button will be aligned at top-left by default. @lukebrandonfarrell
Updating the readme with this change as well and committing.
src/CollageImage.js
Outdated
onPress={this.props.onEditButtonPress} | ||
style={{ | ||
position: "absolute", | ||
margin: 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make the a variable called editButtonIndent
with a default value of 20? (this will allow people to control the button indentation) i.e. margin: this.props.editButtonIndent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be best if we gave an extra option to the developer. Working on this suggestion. @lukebrandonfarrell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukebrandonfarrell I added the editButtonIntent prop with default value of 20. Updating the pull request.
Oh also @jasimawan if you could update the documentation README.md with these features, that would be of great help! :) Working on the replaceImage API now |
I will update the |
@all-contributors please add @jasimawan for code and documentation |
I've put up a pull request to add @jasimawan! 🎉 |
Updated the pull request with all the suggestions and enhancements. Updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me !
I added four new props in the library in CollageImage.js and DynamicCollage.js. This functionality is available only for Dynamic Colleges as StaticCollage doesn't need it. The four new props is:
**- onEditButtonPress: PropTypes.func
onEditButtonPress has a special return statement and it returns matrix used in the collage and index of the picture to be replaced. onEditButtonPress={(matrix, index) => {}} I am attaching the code for onEditButtonPress if anyone will face difficulty to update the state of image in the list.
EditButtonComponent here you can add your custom component or image or icon or anything you want to be displayed on the image. You can style your component as you like. But don't use Button or TouchableOpacity or something like that which have onPress Prop.
editButtonPosition this prop is used to set the alignment of EditButtonComponent. I provided the custom styles for this purpose: top-left | top-right | bottom-left | bottom-right. This prop is mandatory.
isEditButtonVisible in case you don't want to use replace image functionality set it to false. OR If user wants to save his collage image via ViewShot library then you can firstly you can set it to true then when viewshot has been taken change its state to false. So that this button doesn't captured with the collage.
If you want to update anything in this PR. Let me know and i will update it accordingly.
Thanks.