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.

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

All: Resolves #173: Upload attachments > 4 MB when using OneDrive #3195

Merged
merged 11 commits into from Jun 3, 2020
Merged

All: Resolves #173: Upload attachments > 4 MB when using OneDrive #3195

merged 11 commits into from Jun 3, 2020

Conversation

jonath92
Copy link
Contributor

Solves #173. This Issue has been closed by the Bot but only because of inactivity. Also reported in #3060.

@laurent22 laurent22 added the high High priority issues label May 11, 2020
@anjulalk
Copy link
Contributor

Hi,
I don't think requiring fs module would work in the React Native environment.

@jonath92
Copy link
Contributor Author

Thank @anjulalk for reviewing my code. You are probably right. I am new to React and have no experience with React Native. I thought when the code is working on linux it will also work on mobile devices as I didn't made any GUI changes. This was obviously very naive. Sorry for that. I will fix my code but due to the lack of experience it might take some time.

@jonath92
Copy link
Contributor Author

Now it is also working with React-Native. But some Todos for me:

  • What is about CLI client?
  • What is about testing?
  • Solve Todos in code
  • Check if it is working on ios with emulator (I checked with android and Linux. But I currently don't have access to Windows and macOS)

@jonath92
Copy link
Contributor Author

jonath92 commented Jun 1, 2020

I have now tested the Code on Android (on my personal phone which is a Huawei P9 lit with Android 7.0) and Linux (Linux Mint 19.3) and it is working on both without Problems. Also tested with the CliClient without problems. The test of the CliClient doesn't show any failures.

I haven't written any tests as I don't see any tests for Cloud interaction. I haven't tested on Windows and MacOS as I don't have access to a running Windows at the moment (I could test in a VM but as I would need to install Windows first, I would prefer to do it without). I also didn't test on IOS as I have no access to MacOS or IOS (my previous comment was wrong as I didn't know that you need a MacOS to run IOS in an Emulator).

So currently I don't see how I can further improve the code but if you see anything @laurent22 and @anjulalk let me know and I will try my best to fix it.

Copy link
Owner

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request @TheOnlyTrueJonathanHeard, it's going to be useful to many people. Overall there's no major issues, just a few small ones, for which I've added comments. If you need anything to be clarified, please let me know.

ReactNativeClient/lib/fs-driver-node.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/fs-driver-rn.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/onedrive-api.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/onedrive-api.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/file-api-driver-onedrive.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/onedrive-api.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/file-api-driver-onedrive.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/shim.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/shim.js Outdated Show resolved Hide resolved
ReactNativeClient/lib/onedrive-api.js Outdated Show resolved Hide resolved
@jonath92
Copy link
Contributor Author

jonath92 commented Jun 2, 2020

Thanks for reviewing @laurent22 much appreciated. Your comments were very helpful. I think now all your comments are solved.

@jonath92 jonath92 requested a review from laurent22 June 3, 2020 09:19
@laurent22
Copy link
Owner

That looks good, many thanks for this pull request @TheOnlyTrueJonathanHeard!

@laurent22 laurent22 merged commit cfe1911 into laurent22:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high High priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants