-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
I'd be open to keeping this API more programatic like it was... we'd just need to append a hidden input somewhere and then either pass a |
Nice find and solution. The tests from #689 don't apply to mobile web, but I can confirm this fixes the original issue. |
Will await a second review. before merging. Is this still WIP? |
It needs some clean up, but also wanted to make sure everyone agrees on this solution. |
@tgolen can you take a look at this and let me know if it's reasonable? Wondering if there's a simpler way to do what I've done here or if this is OK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to wrap my head around this code, so hang in there with me. This is something new that I haven't really seen before and it's not very easy to follow what's happening, or why it was written this way.
The way I understand it is that iOS Safari needs to have the hidden file input appended directly to the DOM. I'm OK with that. Is there a reason why we can't just do it this way for all platforms? I think that would simplify the code and make it slightly less confusing if it's possible.
@@ -0,0 +1,33 @@ | |||
import PropTypes from 'prop-types'; | |||
import AttachmentPickerNative from '../../libs/AttachmentPickerNative'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was definitely a bit confusing for me at first since I haven't seen this before, but I think I've got it now. I've added a couple of questions just to confirm.
/> | ||
</TouchableOpacity> | ||
<AttachmentPicker> | ||
{({openPicker}) => ( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 expectsthis.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.
There was a problem hiding this comment.
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
class AttachmentPicker extends React.Component { | ||
render() { | ||
return ( | ||
<> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much better 👍 thanks for those changes.
* )} | ||
* </AttachmentPicker> | ||
*/ | ||
class AttachmentPicker extends React.Component { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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();
},
})}
</>
);
};
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Deploying with Cloudflare Pages
|
cc @Jag96 @Julesssss Seems like iOS Safari does not actually fire any change handlers for
<input type="file"/>
unless they are appended to the DOM. We should not really be creating DOM elements on the fly anyway so I came up with a more "React-y" solution which is to use a render prop component that exposes theshow()
method fromAttachmentPicker.show()
and just use that instead of importing the lib? This component will basically append a hidden file input to the DOM alongside whatever stuff it's wrapped in and this way we don't really even needAttachmentPicker
to be cross platform.Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/144167
Tests
Screenshots
❌