Skip to content

Conversation

@marecar3
Copy link
Contributor

@marecar3 marecar3 commented Oct 20, 2020

gutenberg PR: WordPress/gutenberg#26278
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2743

This is a first step on implementing File Block.

This implementation only considers the basics:

  • Insert and render a File Block.
  • Upload a file from the iOS native Documents Picker.
    • Don't let uploading file types not allowed by the blog.
  • Edit the file name and Download button text.

The rest of the features will be implemented in future PRs

CHOOSE A FILE

device-2020-10-27-164047

FILE UPLOADED

device-2020-10-27-164113

To test:

  • Add a File block.
  • Upload a file from the device.
    • Check that you are able to upload images, videos, audio, pdf... but not allowed to upload txt files.
  • Check that you can edit the file name and the Download button text.
  • Check on HTML mode that the HREF is the remote one (not local).
  • Check on preview that you can download the uploaded file.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 20, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 20, 2020

You can test the changes on this Pull Request by downloading the APK here.

@marecar3 marecar3 marked this pull request as draft October 21, 2020 00:17
@jd-alexander
Copy link
Contributor

jd-alexander commented Oct 27, 2020

Hey @marecar3 thanks for these changes. I know this is still marked as a draft but I still took a look 🙏 When I am running this code on my emulator and I try to add a file I get this exception below.

   Caused by: java.lang.IllegalArgumentException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkParameterIsNotNull, parameter mediaUri
        at org.wordpress.android.ui.posts.editor.media.EditorMedia.addNewMediaToEditorAsync(Unknown Source:2)
        at org.wordpress.android.ui.posts.EditPostActivity.onActivityResult(EditPostActivity.java:2663)
        at android.app.Activity.dispatchActivityResult(Activity.java:8310)

@marecar3 marecar3 requested a review from jd-alexander October 27, 2020 20:44
@marecar3 marecar3 added this to the 16.1 milestone Oct 27, 2020
@marecar3 marecar3 marked this pull request as ready for review October 27, 2020 20:46
# Conflicts:
#	libs/gutenberg-mobile
@marecar3
Copy link
Contributor Author

Hey @marecar3 thanks for these changes. I know this is still marked as a draft but I still took a look 🙏 When I am running this code on my emulator and I try to add a file I get this exception below.

   Caused by: java.lang.IllegalArgumentException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkParameterIsNotNull, parameter mediaUri
        at org.wordpress.android.ui.posts.editor.media.EditorMedia.addNewMediaToEditorAsync(Unknown Source:2)
        at org.wordpress.android.ui.posts.EditPostActivity.onActivityResult(EditPostActivity.java:2663)
        at android.app.Activity.dispatchActivityResult(Activity.java:8310)

Thanks, @jd-alexander , I have fixed the issue and I have marked this PR as ready for review.

Copy link
Contributor

@jd-alexander jd-alexander 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 these changes @marecar3 I tested the File block's behavior and it worked as expected. I left a few comments for your consideration. Let me know what you think so that we can proceed. Thank you.

@loremattei loremattei modified the milestones: 16.1 ❄️, 16.2 Nov 2, 2020
@loremattei
Copy link
Contributor

Hey there! I'm moving this to 16.1 because 16.0 has been cut. If you want this to make it to 16.0, please feel free to ping me.

@marecar3
Copy link
Contributor Author

marecar3 commented Nov 2, 2020

Hey @jd-alexander 👋
Thanks for the review.

I have pushed all changes.
Please do another round of testing so that can be sure that I didn't create any regression :) Thanks.

@jd-alexander jd-alexander self-requested a review November 10, 2020 16:05
Copy link
Contributor

@jd-alexander jd-alexander 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 these changes @marecar3 I have tested the changes as described in the PR description and all the functionality that's stated there is working. I have left a few comments for your perusal. Once you have reviewed them then we can finalize this and get it merged in.

Copy link
Contributor

@jd-alexander jd-alexander 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 these changes @marecar3 I have updated the gb-mobile reference and all is well here. I will merge the changes once the build is green.

@marecar3
Copy link
Contributor Author

Thanks for these changes @marecar3 I have updated the gb-mobile reference and all is well here. I will merge the changes once the build is green.

Thanks to you as you did a great job with the review!

@etoledom etoledom changed the base branch from develop to gutenberg/after_1.41.0 November 13, 2020 10:26
@etoledom etoledom merged commit 1848053 into gutenberg/after_1.41.0 Nov 13, 2020
@etoledom etoledom deleted the gutenberg/file-block branch November 13, 2020 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants