Skip to content

Conversation

@etoledom
Copy link
Contributor

@etoledom etoledom commented Oct 19, 2020

Description

Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#2743
WPiOS PR: wordpress-mobile/WordPress-iOS#15122
WPAndroid PR: wordpress-mobile/WordPress-Android#13178

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 features will be implemented in future PRs

EDIT: PICK A FILE has been changed to CHOOSE A FILE.
file_01

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.
    • On the example app, since we can't upload a file, the HREF will remind as local after the fake upload finishes.
  • Check that you can edit the file name and the Download button text.
  • Check on preview that you can download the uploaded file.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Oct 19, 2020
@etoledom etoledom self-assigned this Oct 19, 2020
@github-actions
Copy link

github-actions bot commented Oct 19, 2020

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.91 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.9 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.43 kB 0 B
build/edit-post/style.css 6.42 kB 0 B
build/edit-site/index.js 23.4 kB 0 B
build/edit-site/style-rtl.css 3.95 kB 0 B
build/edit-site/style.css 3.95 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 713 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.77 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.83 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@etoledom etoledom marked this pull request as ready for review October 21, 2020 07:31
@etoledom
Copy link
Contributor Author

@marecar3 - Could you please check the JS side changes? 🙏
@jd-alexander - Could you please check the Android side changes? 🙏
@guarani - Could you please check the iOS side changes? 🙏

Thank you all!

@jd-alexander
Copy link
Contributor

I will be looking into this tomorrow 👍

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.

I did a first pass and the changes look good. I am going to be doing a more detailed run-through on Monday and I will share my findings then 🙏

func presentPicker(origin: UIViewController, filters: [Gutenberg.MediaType], multipleSelection: Bool, callback: @escaping MediaPickerDidPickMediaCallback) {
let uttypeFilters = filters.compactMap { $0.typeIdentifier }
mediaPickerCallback = callback
let docPicker = UIDocumentPickerViewController(documentTypes: uttypeFilters, in: .import)
Copy link
Contributor

Choose a reason for hiding this comment

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

This initializer looks like it was recently deprecated in iOS 14: https://developer.apple.com/documentation/uikit/uidocumentpickerviewcontroller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!
We will need to update WPiOS too at some point 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something best left out of here? Maybe we can create an issue for it and come back later, wdyt?

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

New block! 😀
Left some comments regarding the iOS side of things, will return once the bundles are added here to give it a test on iOS.

@guarani
Copy link
Contributor

guarani commented Oct 26, 2020

Uploading a file from WPiOS is working well ✅

I have some comments and questions:

  1. The icon for the file block is a bit different to other blocks (it's lighter)
    Screenshot screenshot showing file block icon color
  2. The file picker shown here has some differences to what I've seen in other apps. If you compare the file picker seen when running this branch with the one shown in mobile Safari if you upload a file using a File block on the web, the the difference should be apparent. The modal transition is way more fluid when using Safari, and the top navigation bar area of the modal is not grayed out.
    See comparison
    WPiOS Mobile Safari
    Browse
    Recents
  3. When renaming the Download button, for some reason the "Close keyboard" on toolbar button on the right of the toolbar doesn't close the keyboard (it does work if I rename the file name though — it's just the Download button where this doesn't work). This might be an issue with plain text components in general though. Do you know if this is a known issue?
  4. After switching to Dark mode and back to Light mode, the download button background stayed dark:
    See screenshot
  5. The toolbar alignment options are available when editing the Download button title, these don't seem to be working (although the web seems to support them) so I wonder if they belong here?
  6. Same question for the toolbar formatting options when editing a file name
  7. It's possible to @-mention a user from inside the Download button edit text, but instead of adding the @-mention inside the Download button, it is inserted into the file name 🤷
  8. The Download button plain edit text might need a placeholder to ensure it has a minimum width (e.g. at the moment if I delete its text, it's left very narrow and it's hard to tap inside it again to add new text (fwiw the web uses "Add text...")

@etoledom
Copy link
Contributor Author

etoledom commented Oct 27, 2020

Thank you for the detailed review @guarani !

This PR is meant to implement the very basics of the block, most of the points mentioned (and anything outside the bullets to test) are being currently worked on a next PR, or are planed for later:

1 and 4: are design details to be handled later.
3 and 8: will be fixed when using RichText inside the button (there's a small blocker that didn't allow me to use it from the beginning).
5 and 6: Alignment, and all block settings, are part of the next PR.
2: This one is interesting, will check it out 👍
7: I didn't test it, good catch!

Will probably find and implement fixed for 2 and 7 in the following up PRs. If uploads work as expected, I'd prefer to go ahead with the next set of PR if possible :)

@marecar3
Copy link
Contributor

Hey, 👋 @guarani @jd-alexander would you be able to test WPAndroid PR? Thanks.

@jd-alexander
Copy link
Contributor

Hey, 👋 @guarani @jd-alexander would you be able to test WPAndroid PR? Thanks.

Yes of course. I will get it another test 🙏🏾

@guarani
Copy link
Contributor

guarani commented Nov 2, 2020

This PR is meant to implement the very basics of the block, most of the points mentioned (and anything outside the bullets to test) are being currently worked on a next PR, or are planed for later:

@etoledom thanks for clarifying. It's hard to gauge what are known issues and what might have slipped through, so I defaulted to reporting anything I noticed that seemed out of place while testing the basic flow.

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 @etoledom . The code looks good! Once @guarani doesn't have any more suggestions we can get this merged in when the Android changes are finalized.

*/
import styles from './style.scss';

export default class FileEdit extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

@etoledom :) not sure if this was asked elsewhere but I am just checking on the reason we are using a class component instead of a function component since function components lessen the amount of code in a component primarily with the use of hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my starting point as I based it on another block. But I agree we should have this as a function component.
I had this planned after merging the second round of PR (that was when I noticed we should do this). Luckily the rewrites to functions are not complex.

@guarani guarani self-requested a review November 11, 2020 23:38
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

  • 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.
    • On the example app, since we can't upload a file, the HREF will remind as local after the fake upload finishes.
  • Check that you can edit the file name and the Download button text.
  • Check on preview that you can download the uploaded file.

Tested images (HDR photos don't work, but I noticed they're not allowed on the web, so that's probably OK — JPEGs worked), tested video and MOV was greyed out, tested audio (WAV and AIFF) but both appeared greyed out in the iOS file picker, tested PDFs and they worked fine.

Could you please clarify @etoledom if this is expected?

@etoledom
Copy link
Contributor Author

@guarani - The allowed file types depend on the kind of site.
For example, these are the allowed file types for a free WPCom site:

["jpg","jpeg","png","gif","pdf","doc","ppt","odt","pptx","docx","pps","ppsx","xls","xlsx","key","asc"]

wav, aiff, or mov are not allowed here.

For a premium site, these are the allowed filetypes:

["jpg","jpeg","png","gif","pdf","doc","ppt","odt","pptx","docx","pps","ppsx","xls","xlsx","key","asc","mp3","m4a","wav","ogg","zip","ogv","mp4","m4v","mov","wmv","avi","mpg","3gp","3g2"]

We now have wav and mov, but still not aiff.
And testing a wav file on a premium site did work for me.

@guarani guarani self-requested a review November 12, 2020 18:31
Copy link
Contributor

@guarani guarani 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 info @etoledom, looks good to me then.

@etoledom etoledom force-pushed the rnmobile/file-block branch from 8bf08a3 to bfceb2c Compare November 12, 2020 19:18
@etoledom etoledom merged commit ff54fa9 into master Nov 12, 2020
@etoledom etoledom deleted the rnmobile/file-block branch November 12, 2020 22:03
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 12, 2020
@etoledom etoledom mentioned this pull request Nov 13, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants