Skip to content

#195 Add image from camera selection#304

Open
mikolevy wants to merge 7 commits intomasterfrom
195-add-image-selection
Open

#195 Add image from camera selection#304
mikolevy wants to merge 7 commits intomasterfrom
195-add-image-selection

Conversation

@mikolevy
Copy link
Contributor

Not the greatest solution however should unblock a few tasks. During workin on this some problems with selecting from device appears - to unblock this part I extracted a separated task #290. We will need to cover things like loading images in the future (loading bar or even preload all images on plan start to improve user experience). Another thing is #291 which blocked me from testing image displaying on task edition and image reselection.

Copy link
Contributor

@bklukaczewski bklukaczewski left a comment

Choose a reason for hiding this comment

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

Left some comments

Firebase version is most important as it can lead to more issues. The rest is just "nice to have" :)

},
"dependencies": {
"@react-native-community/slider": "^2.0.8",
"@react-native-firebase/app": "^6.4.0",
Copy link
Contributor

@bklukaczewski bklukaczewski Jun 15, 2020

Choose a reason for hiding this comment

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

We use v5 of firebase library already. We shouldn't mix it with v6

the best option would be to migrate to v6 ;) but that's a separate task

"react-native-floating-action": "^1.19.1",
"react-native-gesture-handler": "^1.5.6",
"react-native-image-crop-picker": "^0.26.2",
"react-native-image-picker": "^2.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to use react-native-image-crop-picker as it's already on the dependency list

import uuid from 'react-native-uuid';
import { getImagesStorage } from '../models/FirebaseRefProxy';

export const uploadImage = async (imageUri: string, fileName: string | undefined): Promise<string> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if fileName can be undefined, we can make it optional parameter fileName?

export const uploadImage = async (imageUri: string, fileName: string | undefined): Promise<string> => {
const DEFAULT_EXTENSION = '.jpg';
const fileExtension = fileName ? fileName.split('.').pop() : DEFAULT_EXTENSION;
const uploadedFileName = `${uuid.v1()}.${fileExtension}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Firebase would generate filename for us :)

Copy link
Contributor

@pwysowski pwysowski left a comment

Choose a reason for hiding this comment

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

I think you should consider all comments added by Bartek and then you should merge ;)

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.

3 participants