Open filip-solecki opened 3 weeks ago
@Pujan92 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]
@grgia @Guccio163 This is the new PR for Share Extension in ND, I'm linking also the old one to keep everything here
@grgia can you designate someone to do the review?
Videos look pretty good to me.
It's been a while since we wrote the design doc for this, but I think we should rename "Submit" here to just "Create expense":
Thoughts @Expensify/design @JmillsExpensify @trjExpensify?
Also, for this submit flow, how does it work exactly? Does it just automatically scan the image, or something else? I am wondering why there aren't more fields for things like Merchant on the confirmation page.
Why is there a small delay between having just one Share button and then two buttons?
Posted in #contributor-plus for a volunteer
π§ @mountiny has triggered a test hybrid app build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
β FAILED β | β FAILED β | |
The QR code can't be generated, because the Android build failed | The QR code can't be generated, because the iOS build failed | |
Desktop :computer: | Web :spider_web: | |
N/A | N/A | |
N/A | N/A |
:eyes: View the workflow run that generated this build :eyes:
π§ @grgia has triggered a test build. You can view the workflow run here.
π§ @mountiny has triggered a test hybrid app build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55748/index.html | β FAILED β | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55748/NewExpensify.dmg | https://55748.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
Thoughts @Expensify/design @JmillsExpensify @trjExpensify?
Agreed, Create
would be consistent now.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55748/index.html | β FAILED β | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
N/A | N/A | |
N/A | N/A |
:eyes: View the workflow run that generated this build :eyes:
Why is there a small delay between having just one Share button and then two buttons?
Currently we can share also other types than just images, but only images may go to Submit section, the delay is because by default we show only Share tab (all file types might be shared) but after file type check we decide if we should show also Submit Tab for creating an expense. Should we hide everything by default and show when we know which Tabs should be available? @shawnborton @trjExpensify
Also, for this submit flow, how does it work exactly? Does it just automatically scan the image, or something else? I am wondering why there aren't more fields for things like Merchant on the confirmation page.
This works exactly the same as creating an expense with the β+β button, the image is scanned automatically
@filip-solecki Can you fix the build error on iOS?
Currently we can share also other types than just images, but only images may go to Submit section, the delay is because by default we show only Share tab (all file types might be shared) but after file type check we decide if we should show also Submit Tab for creating an expense. Should we hide everything by default and show when we know which Tabs should be available? @shawnborton @trjExpensify
I wonder if by default we should show both options with the Create
option disabled until we knowβthen after the file type check all we'd have to do is "un-disable" the Create tab. Might be a little less jarring since the UI won't need to shift at all. Thoughts on that approach @Expensify/design @trjExpensify?
Yeah maybe, or what about skeleton loading the button area until we know what options we can present you?
I like the skeleton idea if it's not a pain to implement, otherwise I would be fine just leaving things as they are given that this isn't a mainline flow?
I like the skeleton idea if it's not a pain to implement, otherwise I would be fine just leaving things as they are given that this isn't a mainline flow?
+1. I like the skeleton idea too.
+3 on the skellies.
@filip-solecki Is there something you are stuck here on?
I got stuck on merging SmartScan from OD with new Share Extension, but it is going pretty well today, I think I will add skeleton tomorrow and I'll take a look at the provisioning profile
@filip-solecki how's it going?
I've prepared branch with changes needed for HybridApp and I've added skeleton for the Tab Navigator, WDYT? @Expensify/design @grgia
Which one looks better? Or maybe you thought about something else π
It looks like the button shapes aren't perfectly pill shaped, they should have a larder border radius I think:
For the text in the middle:
I wouldn't think they would be rounded. I would think that they would be rectangular bars like we do elsewhere:
@filip-solecki I pushed a PR with the encrypted provisioning profile here https://github.com/Expensify/App/pull/56267
@grgia can you rerun standalone ND build?
π§ @grgia has triggered a test build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55748/index.html | β FAILED β | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55748/NewExpensify.dmg | https://55748.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
How about this @shawnborton ?
I think that feels better. Would love a video to see the loading in action though, if you get a moment. Thanks!
I think that feels better. Would love a video to see the loading in action though, if you get a moment. Thanks!
Same. I also think we should decrease the gap between the skeleton buttons to be closer to what they'll end up being (8px).
Oh that's a fair shout, we actually don't really ever show two filled buttons side-by-side given that its a segmented control:
So I wonder if we should only put a background color behind the left button in the skeleton? And then there would be zero gap between the two.
Oh yeah I didn't realize that. That's a great call.
So I wonder if we should only put a background color behind the left button in the skeleton? And then there would be zero gap between the two
Super down with this approach.
@shawnborton @dannymcclain could we mock that up?
I'm thinking something like this:
cc @Expensify/design for viz
Looks good to me!
π§ @grgia has triggered a test build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55748/index.html | β FAILED β | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55748/NewExpensify.dmg | https://55748.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
π§ @grgia has triggered a test build. You can view the workflow run here.
π§ @grgia has triggered a test hybrid app build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
β FAILED β | β FAILED β | |
The QR code can't be generated, because the Android build failed | The QR code can't be generated, because the iOS build failed | |
Desktop :computer: | Web :spider_web: | |
N/A | N/A | |
N/A | N/A |
:eyes: View the workflow run that generated this build :eyes:
Late, but like the new skelly
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55748/index.html | β FAILED β | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55748/NewExpensify.dmg | https://55748.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
π§ @AndrewGable has triggered a test build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
β FAILED β | β FAILED β | |
The QR code can't be generated, because the Android build failed | The QR code can't be generated, because the iOS build failed | |
Desktop :computer: | Web :spider_web: | |
β FAILED β | https://55748.pr-testing.expensify.com | |
The QR code can't be generated, because the Desktop build failed |
:eyes: View the workflow run that generated this build :eyes:
@staszekscp let's merge main and try again
π§ @AndrewGable has triggered a test build. You can view the workflow run here.
Details
This PR introduces the implementation of Share Extension for mobile apps both for Android and iOS.
MOBILE-EXPENSIFY:13370
Fixed Issues
$https://github.com/Expensify/App/issues/48788 $https://github.com/Expensify/App/issues/48789 PROPOSAL:
Tests
Share test:
Submit test:
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps./** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Screenshots/Videos
Android: Native
Share: https://github.com/user-attachments/assets/c02605d4-b7d3-448e-8797-0f32ca98381a Submit: https://github.com/user-attachments/assets/bbee5791-6c04-49f3-b551-609ecf0b5b1fAndroid: mWeb Chrome
N/AiOS: Native
Share: https://github.com/user-attachments/assets/119da39e-323f-4cd2-a39a-94fe5ac3dc42 Submit: https://github.com/user-attachments/assets/671eb9d0-7275-4470-bcad-50a50e1cd53aiOS: mWeb Safari
N/AMacOS: Chrome / Safari
N/AMacOS: Desktop
N/A