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

marcaaron · 2020-10-28T21:43:02Z

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 the show() method from AttachmentPicker.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 need AttachmentPicker to be cross platform.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/144167

Tests

Test that files can be attached via Safari on iOS
Follow tests in Add document picking support to mobile #689 and make sure they still pass

Screenshots

❌

marcaaron · 2020-10-28T21:49:34Z

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 ref or use document.getElementById to target it or whatever. But I sort of like how this keeps everything in React land.

Julesssss · 2020-10-29T13:50:53Z

Nice find and solution.

The tests from #689 don't apply to mobile web, but I can confirm this fixes the original issue.

Julesssss · 2020-10-29T13:51:59Z

Will await a second review. before merging. Is this still WIP?

marcaaron · 2020-10-29T15:38:29Z

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.

marcaaron · 2020-10-29T15:54:29Z

@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.

tgolen

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.

tgolen · 2020-10-29T16:10:35Z

src/components/AttachmentPicker/index.native.js

@@ -0,0 +1,33 @@
+import PropTypes from 'prop-types';
+import AttachmentPickerNative from '../../libs/AttachmentPickerNative';


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.

Jag96

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.

Jag96 · 2020-10-29T17:49:14Z

src/pages/home/report/ReportActionCompose.js

- />
- </TouchableOpacity>
+ <AttachmentPicker>
+ {({openPicker}) => (


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?

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.

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

Jag96 · 2020-10-29T17:52:33Z

src/components/AttachmentPicker/index.js

+class AttachmentPicker extends React.Component {
+ render() {
+ return (
+ <>


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.

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.

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.

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

Jag96

LGTM

tgolen

This looks much better 👍 thanks for those changes.

tgolen · 2020-11-04T19:44:10Z

src/components/AttachmentPicker/index.js

+ * )}
+ * </AttachmentPicker>
+ */
+class AttachmentPicker extends React.Component {


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)

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(); }, })} </> ); };

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

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

cloudflare-workers-and-pages · 2023-10-25T15:56:58Z

Deploying with Cloudflare Pages

Latest commit:	`734f9e4`
Status:	✅ Deploy successful!
Preview URL:	https://146b22df.helpdot.pages.dev

View logs

Make AttachmentPicker a component to fix Safari

fad3917

marcaaron requested a review from AndrewGable as a code owner October 28, 2020 21:43

marcaaron self-assigned this Oct 28, 2020

fix style

2854e9f

newline

d2659e0

Julesssss previously approved these changes Oct 29, 2020

View reviewed changes

marcaaron added 3 commits October 29, 2020 08:42

Use a function component

e27da2b

add display name

d79269d

add example in the doc

b20f520

marcaaron dismissed Julesssss’s stale review via b20f520 October 29, 2020 15:46

marcaaron changed the title ~~[WIP] Make AttachmentPicker a component instead of a lib to fix Safari~~ Make AttachmentPicker a component instead of a lib to fix Safari Oct 29, 2020

js-doc fix

8ea88a2

marcaaron requested a review from tgolen October 29, 2020 15:53

Julesssss previously approved these changes Oct 29, 2020

View reviewed changes

tgolen requested changes Oct 29, 2020

View reviewed changes

remove createRef use instance

b2a9203

marcaaron dismissed Julesssss’s stale review via b2a9203 October 29, 2020 17:05

marcaaron added 2 commits October 29, 2020 10:27

Move methods into AttachmentPicker component

df66da1

fix up names

707c44c

Jag96 reviewed Oct 29, 2020

View reviewed changes

Jag96 approved these changes Nov 3, 2020

View reviewed changes

marcaaron requested a review from tgolen November 4, 2020 16:18

tgolen requested changes Nov 4, 2020

View reviewed changes

tgolen approved these changes Nov 9, 2020

View reviewed changes

tgolen merged commit 0b00f3a into master Nov 9, 2020

tgolen deleted the marcaaron-fixSafariPicker2 branch November 9, 2020 17:18

Taimoor0217 mentioned this pull request Nov 19, 2023

[Snyk] Security upgrade react-native-pdf from 6.6.2 to 6.7.2 Taimoor0217/App#18

Open

osama256 mentioned this pull request Nov 20, 2023

[Snyk] Security upgrade react-native-pdf from 6.6.2 to 6.7.2 osama256/App#16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcaaron commented Oct 28, 2020

marcaaron commented Oct 28, 2020

Julesssss commented Oct 29, 2020

Julesssss commented Oct 29, 2020 •

edited

Loading

marcaaron commented Oct 29, 2020

marcaaron commented Oct 29, 2020

tgolen left a comment

tgolen Oct 29, 2020

Jag96 left a comment

Jag96 Oct 29, 2020

marcaaron Oct 29, 2020

Jag96 Oct 29, 2020

Jag96 Oct 29, 2020

marcaaron Oct 29, 2020

marcaaron Oct 29, 2020

Jag96 Oct 29, 2020

Jag96 left a comment

tgolen left a comment

tgolen Nov 4, 2020

marcaaron Nov 5, 2020 •

edited

Loading

marcaaron Nov 5, 2020 •

edited

Loading

tgolen Nov 9, 2020

cloudflare-workers-and-pages bot commented Oct 25, 2023 •

edited

Loading

		@@ -0,0 +1,33 @@
		import PropTypes from 'prop-types';
		import AttachmentPickerNative from '../../libs/AttachmentPickerNative';

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

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

Conversation

marcaaron commented Oct 28, 2020

Fixed Issues

Tests

Screenshots

marcaaron commented Oct 28, 2020

Julesssss commented Oct 29, 2020

Julesssss commented Oct 29, 2020 • edited Loading

marcaaron commented Oct 29, 2020

marcaaron commented Oct 29, 2020

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jag96 left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

marcaaron Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Oct 25, 2023 • edited Loading

Deploying with Cloudflare Pages

Julesssss commented Oct 29, 2020 •

edited

Loading

marcaaron Nov 5, 2020 •

edited

Loading

marcaaron Nov 5, 2020 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 25, 2023 •

edited

Loading