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

Make AttachmentPicker a component instead of a lib to fix Safari #721

Merged
merged 10 commits into from
Nov 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/components/AttachmentPicker/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import React from 'react';
import PropTypes from 'prop-types';

const propTypes = {
children: PropTypes.func.isRequired,
};

/**
* This component renders a function as a child and
* returns a "show attachment picker" method that takes
* a callback. This is the web/mWeb/desktop version since
* on iOS Safari we must append a hidden input to the DOM
* and listen to onChange event. When the show method is
* called an attachment
*
* @example
* <AttachmentPicker>
* {({openPicker}) => (
* <Button
* onPress={() => {
* openPicker({
* onPicked: (file) => {
* // Display or upload File
* },
* });
* }}
* />
* )}
* </AttachmentPicker>
*/
class AttachmentPicker extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be switched to a functional component now? I think that this.onPicked is the only thing stopping it, but I think this might work:

(props) => {
   let onPicked;
   return ( blah blah );
}

(Let me know if that doesn't make sense)

Copy link
Contributor Author

@marcaaron marcaaron Nov 5, 2020

Choose a reason for hiding this comment

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

It does, but we also have both a ref and an instance field to store onPicked. So, I can make this a function, but will need to use a hook called useRef() and it will end up looking like this...

const AttachmentPicker = props => {
    const fileInput = useRef(null);
    const onPickedCallback = useRef(null);
    return (
        <>
            <input
                hidden
                type="file"
                ref={fileInput}
                onChange={(e) => {
                    const file = e.target.files[0];
                    file.uri = URL.createObjectURL(file);
                    onPickedCallback.current(file);
                }}
            />
            {props.children({
                openPicker: ({onPicked}) => {
                    onPickedCallback.current = onPicked;
                    fileInput.current.click();
                },
            })}
        </>
    );
};

Copy link
Contributor Author

@marcaaron marcaaron Nov 5, 2020

Choose a reason for hiding this comment

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

The reason why your example won't work is because let onPicked cannot be updated in the render cycle. Which is what hooks are for they allow you to store values (and do other junk) from one render to the next --> https://reactjs.org/docs/hooks-reference.html#useref

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks for explaining and exploring that. Let's just leave it the way it is for now.

render() {
return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this here? I read the fragments doc (this is still new to me) but I'm still not sure why it'd be required in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly to keep the component view hierarchy clean. We could make this a View but then we have an extra thing that we don't need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By convention we can only return a single root element or an array from any react component. Using arrays is problematic since you must then add keys to everything when they might not ever change. And wrapping things with "container" elements just makes markup heavy for no reason.

Fragment is an escape hatch for that React convention. So, sort of a way of flagging things and telling react that it's OK to render this as a sibling to whatever comes after it and we don't need any keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah perfect, that's the context I was missing, thanks for the explanation!

<input
hidden
type="file"
ref={el => this.fileInput = el}
onChange={(e) => {
const file = e.target.files[0];
file.uri = URL.createObjectURL(file);
this.onPicked(file);
}}
/>
{this.props.children({
openPicker: ({onPicked}) => {
this.onPicked = onPicked;
this.fileInput.click();
},
})}
</>
);
}
}

AttachmentPicker.propTypes = propTypes;
export default AttachmentPicker;
117 changes: 117 additions & 0 deletions src/components/AttachmentPicker/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/**
* The react native image/document pickers work for iOS/Android, but we want to wrap them both within AttachmentPicker
*/
import RNImagePicker from 'react-native-image-picker';
import RNDocumentPicker from 'react-native-document-picker';
import PropTypes from 'prop-types';

/**
* See https://github.com/react-native-community/react-native-image-picker/blob/master/docs/Reference.md#options
* for ImagePicker configuration options
*/
const imagePickerOptions = {
title: 'Select an Attachment',
takePhotoButtonTitle: 'Take Photo',
chooseFromLibraryButtonTitle: 'Choose from Gallery',
customButtons: [{name: 'Document', title: 'Choose Document'}],
storageOptions: {
skipBackup: true,
},
};

/**
* See https://github.com/rnmods/react-native-document-picker#options for DocumentPicker configuration options
*/
const documentPickerOptions = {
type: [RNDocumentPicker.types.allFiles],
};

/**
* Launch the DocumentPicker. Results are in same format as ImagePicker, so we can pass the repsonse to the
* callback as is.
*
* @param {Object} callback
*/
function showDocumentPicker(callback) {
RNDocumentPicker.pick(documentPickerOptions).then((results) => {
callback(results);
}).catch((error) => {
if (!RNDocumentPicker.isCancel(error)) {
throw error;
}
});
}

/**
* Launch the AttachmentPicker. We display the ImagePicker first, as the document option is displayed as a
* custom ImagePicker list item.
*
* @param {Object} callback
*/
function show(callback) {
RNImagePicker.showImagePicker(imagePickerOptions, (response) => {
if (response.error) {
console.error(`Error during attachment selection: ${response.error}`);
} else if (response.customButton) {
showDocumentPicker(callback);
} else if (!response.didCancel) {
callback(response);
}
});
}

/**
* The data returned from `show` is different on web and mobile, so use this function to ensure the data we
* send to the xhr will be handled properly.
*
* @param {Object} fileData
* @return {Object}
*/
function getDataForUpload(fileData) {
return {
name: fileData.fileName || 'chat_attachment',
type: fileData.type,
uri: fileData.uri,
};
}

const propTypes = {
children: PropTypes.func.isRequired,
};

/**
* This component renders a function as a child and
* returns a "show attachment picker" method that takes
* a callback. This is the web/mWeb/desktop version since
* on iOS Safari we must append a hidden input to the DOM
* and listen to onChange event. When the show method is
* called an attachment
*
* @example
* <AttachmentPicker>
* {({openPicker}) => (
* <Button
* onPress={() => {
* openPicker({
* onPicked: (file) => {
* // Display or upload File
* },
* });
* }}
* />
* )}
* </AttachmentPicker>
*
* @returns {Function}
*/
const AttachmentPicker = ({children}) => children({
openPicker: ({onPicked}) => {
show((response) => {
onPicked(getDataForUpload(response));
});
},
});

AttachmentPicker.propTypes = propTypes;
AttachmentPicker.displayName = 'AttachmentPicker';
export default AttachmentPicker;
31 changes: 0 additions & 31 deletions src/libs/AttachmentPicker/index.js

This file was deleted.

75 changes: 0 additions & 75 deletions src/libs/AttachmentPicker/index.native.js

This file was deleted.

53 changes: 24 additions & 29 deletions src/pages/home/report/ReportActionCompose.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import TextInputFocusable from '../../../components/TextInputFocusable';
import sendIcon from '../../../../assets/images/icon-send.png';
import IONKEYS from '../../../IONKEYS';
import paperClipIcon from '../../../../assets/images/icon-paper-clip.png';
import AttachmentPicker from '../../../libs/AttachmentPicker';
import AttachmentPicker from '../../../components/AttachmentPicker';
import withIon from '../../../components/withIon';
import {addAction, saveReportComment, broadcastUserIsTyping} from '../../../libs/actions/Report';
import ReportTypingIndicator from './ReportTypingIndicator';
Expand Down Expand Up @@ -37,7 +37,6 @@ class ReportActionCompose extends React.Component {
this.submitForm = this.submitForm.bind(this);
this.triggerSubmitShortcut = this.triggerSubmitShortcut.bind(this);
this.submitForm = this.submitForm.bind(this);
this.showAttachmentPicker = this.showAttachmentPicker.bind(this);
this.setIsFocused = this.setIsFocused.bind(this);
this.comment = '';
this.state = {
Expand Down Expand Up @@ -128,22 +127,6 @@ class ReportActionCompose extends React.Component {
this.setTextInputShouldClear(true);
}

/**
* Handle the attachment icon being tapped
*
* @param {SyntheticEvent} e
*/
showAttachmentPicker(e) {
e.preventDefault();

AttachmentPicker.show((response) => {
console.debug(`Attachment selected: ${response.uri}, ${response.type}, ${response.name}, ${response.size}`);

addAction(this.props.reportID, '', AttachmentPicker.getDataForUpload(response));
this.textInput.focus();
});
}

render() {
return (
<View style={[styles.chatItemCompose]}>
Expand All @@ -153,17 +136,29 @@ class ReportActionCompose extends React.Component {
styles.flexRow
]}
>
<TouchableOpacity
onPress={this.showAttachmentPicker}
style={[styles.chatItemAttachButton]}
underlayColor={colors.componentBG}
>
<Image
style={[styles.chatItemSubmitButtonIcon]}
resizeMode="contain"
source={paperClipIcon}
/>
</TouchableOpacity>
<AttachmentPicker>
{({openPicker}) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand this: this code is so that the component knows that everything below (wrapped) is to be set in the children prop right? And it also lets us map openPicker below to openPicker in the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sounds like you've got it.

Here's how I would explain it...

  • AttachmentPicker is a component that expects this.props.children to be function that ultimately returns a block of react stuff to render (and also calls that function in its render method)
  • In this case the function is {({openPicker}) => (... render react stuff)}
  • And the beauty of that is that we can encapsulate whatever kind of functionality we want in AttachmentPicker and allow it to be used in the react stuff the function renders

This way the AttachmentPicker has no knowledge about what uses it. Which is useful since whenever we need to use it we need to inject the UI with an input tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, I like the approach! The @example docs also makes this easier to follow

<TouchableOpacity
onPress={(e) => {
e.preventDefault();
openPicker({
onPicked: (file) => {
addAction(this.props.reportID, '', file);
this.setTextInputShouldClear(true);
},
});
}}
style={[styles.chatItemAttachButton]}
underlayColor={colors.componentBG}
>
<Image
style={[styles.chatItemSubmitButtonIcon]}
resizeMode="contain"
source={paperClipIcon}
/>
</TouchableOpacity>
)}
</AttachmentPicker>
<TextInputFocusable
multiline
textAlignVertical="top"
Expand Down