Add attachment button link that links to expensify.com report #399
Conversation
|
Mind sharing a screenshot of how this looks so far? Let me know how we can help, too. I'm hoping that we can do something similar to what we've been mocking up where the attachment icon is off to the left: |
Jag96
changed the title
|
Got the CSS working to have the attachment button on the left, this is ready for review |
|
Actually I wonder if we want to remove the right border given that the input can grow so much? Maybe that will look a little less weird? |
|
I agree it looks less awkward without the border when there's a lot of text
Should I kill the border and update the spacing between the attachment icon and the text field as well? |
|
Actually let me pull your branch down quickly and see how it feels, but I think the spacing might be okay? |
|
Updated the screenshots w/ the latest design changes 👍 |
|
bump @marcaaron |
| href, | ||
| ...props | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| }) => (<TouchableOpacity onPress={() => window.open(href, '_blank')} {...props} />); |
There was a problem hiding this comment.
I initially wanted to pull the "You Aren't Gonna Need It" alarm on this one since we're only using this component once. But then saw why we were specializing here (i.e. the native/web implementations).
Since we really only need the onPress behavior to be cross-platform why not use a utility method to achieve the same thing? This way we can prefer to use the regular TouchableOpacity and pass the href to that method via its parent?
Another alternative would be to use a render prop or HOC, but I think it's overkill for this case.
<TouchableOpacity onPress={() => openLinkInNewTab(props.href)}/>There was a problem hiding this comment.
I think it's more likely that we'll get reuse out of a simple function vs. a React component that has a hardcoded event handler.
There was a problem hiding this comment.
That makes sense to me, I'll update this use a utility lib 👍
|
Tests well for me :) |
|
Updated this to use a utility lib, @marcaaron lmk if this is more in line with what you were thinking |
|
Not sure if we want to get to the bottom of this style issue but this is what it looks like on Android otherwise tests well for me and it's NAB so feel free to merge.
|
| * @param {String} href | ||
| */ | ||
| const openURLInNewTab = (href) => { | ||
| window.open(href, '_blank'); |
There was a problem hiding this comment.
Should we refactor the <Anchor> component to use openURLInNewTab? It has the same idea where we have to use window.open on web and Linking for native.
cc @marcaaron or @tgolen for thoughts
There was a problem hiding this comment.
Yeah! Do they differ in any other way besides this method? I think that's a good case for eliminating two separate implementations by creating a shared behavior instead of components that specialize in a behavior.
There was a problem hiding this comment.
Tested it out and it works! I'll remove the Anchor component and add some tests to this PR confirm the behavior
src/style/StyleSheet.js
Outdated
| @@ -544,10 +544,14 @@ const styles = { | |||
|
|
|||
| textInputCompose: { | |||
| borderWidth: 0, | |||
| borderRadius: 0, | |||
| outline: 0, | |||
There was a problem hiding this comment.
I know you didn't add this, but can you remove outline? It's throwing errors on native platforms.
There was a problem hiding this comment.
I think this needs to be outlineWidth actually, right? I remember looking into this at one point
There was a problem hiding this comment.
I think it's used in one other spot too, let's remove both instances
There was a problem hiding this comment.
Actually here is an issue where they discuss it: necolas/react-native-web#48
So it looks like it is supported on RNW, but throws on RN 🤔
There was a problem hiding this comment.
Yeah, looks like outlineWidth: 0 prevents the outline on the textbox and also doesn't throw an error
|
Updated to remove the Anchor component and update outline to outlineWidth, ready for another look |
This PR adds the workaround mentioned in #87 (comment) so that users using Expensify Chat can have an easier way of uploading photos.
This workaround is temporary and will be replaced with attachment upload functionality described in #87 (comment). The reason we're adding this workaround first is so that we can get everybody moved over to using DMs on chat.expensify.com ASAP
Tests
Screenshots
Note the placeholder alignment on Android is an existing issue