Skip to content

#195 Add image from camera selection - #304

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

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

Conversation

@mikolevy

Copy link
Copy Markdown
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.

@bklukaczewski bklukaczewski left a comment

Copy link
Copy Markdown
Contributor

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" :)

Comment thread package.json
},
"dependencies": {
"@react-native-community/slider": "^2.0.8",
"@react-native-firebase/app": "^6.4.0",

@bklukaczewski bklukaczewski Jun 15, 2020

Copy link
Copy Markdown
Contributor

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

Comment thread package.json
"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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 :)

@pwysowski pwysowski left a comment

Copy link
Copy Markdown
Contributor

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