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

SetNodePicture: Refactor 'handleImageTap' #2100

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

shubhamkmr04
Copy link
Contributor

@shubhamkmr04 shubhamkmr04 commented Mar 30, 2024

No description provided.

@@ -125,35 +125,14 @@ export default class SetNodePicture extends React.Component<
);
};

getPhoto(photo: string | null): string {
getPhoto(photo: string | null): object | string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was it necessary to change the structure of the getPhoto calls? Seems like we could have just removed handleImageTap and made the change on line 194

Copy link
Contributor Author

@shubhamkmr04 shubhamkmr04 Mar 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did that coz we needed the object to return with the key uri in line 131, to fetch gallery imported photos

@kaloudis
Copy link
Contributor

This approach doesn't work. Images are lost on reload

@kaloudis
Copy link
Contributor

What if we refactor handleImageTap to just save the image uri?

handleImageTap = async (item) => {
        try {
            // Fetch the local image
            const imageUri = Image.resolveAssetSource(item).uri;
            this.setState({ photo: imageUri });
        } catch (error) {
            console.error('Error resolving image uri:', error);
        }
    };

@shubhamkmr04
Copy link
Contributor Author

What if we refactor handleImageTap to just save the image uri?

handleImageTap = async (item) => {
        try {
            // Fetch the local image
            const imageUri = Image.resolveAssetSource(item).uri;
            this.setState({ photo: imageUri });
        } catch (error) {
            console.error('Error resolving image uri:', error);
        }
    };

yup we can do that

@shubhamkmr04 shubhamkmr04 force-pushed the shubham/node-picture-bug branch from 59a00ba to 478a0f3 Compare March 31, 2024 10:34
@shubhamkmr04 shubhamkmr04 changed the title Node Pictures: Save images location directly and remove 'handleImageTap' Node Pictures: Saving images uri Mar 31, 2024
@kaloudis
Copy link
Contributor

Holds up against Android upgrades. About to test iOS

Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not hold up on iOS, I believe the file paths change every upgrade.

We'll have to emulate what we do on custom images and save a copy of the image that we point to.

@shubhamkmr04 shubhamkmr04 changed the title Node Pictures: Saving images uri SetNodePicture: Refactor 'handleImageTap' Apr 1, 2024
@kaloudis
Copy link
Contributor

kaloudis commented Apr 1, 2024

The file storage is working correctly now, however the approach isn't quite right.

We are unnecessarily creating new copies of each image every time we tap it. The 'download' event should only trigger once the Choose Picture button is pressed

@shubhamkmr04 shubhamkmr04 force-pushed the shubham/node-picture-bug branch from eead25b to 20841b4 Compare April 2, 2024 15:00
@shubhamkmr04 shubhamkmr04 requested a review from kaloudis April 2, 2024 15:01
Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing all my tests. nice work

@kaloudis kaloudis merged commit ad972fb into ZeusLN:master Apr 2, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants