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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 10 commits into from
Nov 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fad3917
Make AttachmentPicker a component to fix Safari
marcaaron Oct 28, 2020
2854e9f
fix style
marcaaron Oct 28, 2020
d2659e0
newline
marcaaron Oct 28, 2020
e27da2b
Use a function component
marcaaron Oct 29, 2020
d79269d
add display name
marcaaron Oct 29, 2020
b20f520
add example in the doc
marcaaron Oct 29, 2020
8ea88a2
js-doc fix
marcaaron Oct 29, 2020
b2a9203
remove createRef use instance
marcaaron Oct 29, 2020
df66da1
Move methods into AttachmentPicker component
marcaaron Oct 29, 2020
707c44c
fix up names
marcaaron Oct 29, 2020
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
39 changes: 39 additions & 0 deletions src/components/AttachmentPicker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React, {createRef} from 'react';
import PropTypes from 'prop-types';

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

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 •
edited
Loading

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 •
edited
Loading

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.

constructor(props) {
super(props);
this.onChangeCallback = createRef();
}

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.onChangeCallback.current(file);
}}
/>
{this.props.children({
show: (callback) => {
this.onChangeCallback.current = callback;
this.fileInput.click();
},
})}
</>
);
}
}

AttachmentPicker.propTypes = propTypes;
export default AttachmentPicker;
27 changes: 27 additions & 0 deletions src/components/AttachmentPicker/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import React, {createRef} from 'react';
import PropTypes from 'prop-types';
import AttachmentPickerNative from '../../libs/AttachmentPickerNative';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking the file naming conventions of the repo. What I would probably suggest is that if this is the only file that imports AttachmentPickerNative, then I would just move all the logic from AttachmentPickerNative.js into this file directly, so that it remains in an index.native.js file.


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

class AttachmentPicker extends React.Component {
constructor(props) {
super(props);
this.onChangeCallback = createRef();
}

render() {
return this.props.children({
show: (callback) => {
AttachmentPickerNative.show((response) => {
callback(AttachmentPickerNative.getDataForUpload(response));
});
},
});
}
}

AttachmentPicker.propTypes = propTypes;
export default AttachmentPicker;
31 changes: 0 additions & 31 deletions src/libs/AttachmentPicker/index.js
View file
Open in desktop

This file was deleted.

0 src/libs/AttachmentPicker/index.native.js → src/libs/AttachmentPickerNative.js
View file
Open in desktop
File renamed without changes.
51 changes: 22 additions & 29 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
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,27 @@ 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>
{({show}) => (
<TouchableOpacity
onPress={(e) => {
e.preventDefault();
show((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