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.

Sign up for GitHub

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

Jump to bottom

[RNMobile] Adding WPMediaLibrary source to File block #26960

Merged
merged 9 commits into from
Nov 25, 2020
Merged

[RNMobile] Adding WPMediaLibrary source to File block #26960

merged 9 commits into from
Nov 25, 2020

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Nov 13, 2020 •
edited
Loading

Description

gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2805
WPiOS: wordpress-mobile/WordPress-iOS#15291
WPAndroid: wordpress-mobile/WordPress-Android#13413

This PR adds the WordPress Media Library as a media source for File block.

How has this been tested?

  • Run the example app.
  • Add a file block.
  • Press on Choose a file.
    • Check that the option WordPress Media Library is present.
  • Select such option.
    • Check that WordPress.zip file has been added to the block.
    • (This is mocked on the example app).

Screenshots

Screenshot 2020-11-18 at 11 05 18

Types of changes

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 Nov 13, 2020
@etoledom etoledom self-assigned this Nov 13, 2020
@github-actions
Copy link

github-actions bot commented Nov 13, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 1.2 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 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.72 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 134 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.95 kB 0 B
build/block-library/editor.css 8.95 kB 0 B
build/block-library/index.js 148 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 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.1 kB 0 B
build/components/index.js 172 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.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 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.95 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.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/index.js 23.6 kB 0 B
build/edit-site/style-rtl.css 3.92 kB 0 B
build/edit-site/style.css 3.92 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 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 43.3 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.27 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 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 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.81 kB 0 B
build/nux/index.js 3.42 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.84 kB 0 B
build/reusable-blocks/index.js 2.92 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.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

This was referenced Nov 13, 2020
Merged
Merged
@jd-alexander jd-alexander self-requested a review November 17, 2020 03:07
@etoledom etoledom force-pushed the rnmobile/file-block-II branch from e777c4b to cbd7358 Compare November 17, 2020 07:40
Base automatically changed from rnmobile/file-block-II to master November 17, 2020 20:46
@etoledom etoledom force-pushed the rnmobile/file-block-III-wp-media-library branch from d5a7304 to 92220ed Compare November 17, 2020 21:29
@jd-alexander jd-alexander mentioned this pull request Nov 18, 2020
Merged
3 tasks
@etoledom etoledom changed the title [RNMoobile] [WIP] Adding WPMediaLibrary source to File block [RNMoobile] Adding WPMediaLibrary source to File block Nov 18, 2020
@etoledom etoledom marked this pull request as ready for review November 18, 2020 10:08
@etoledom etoledom requested a review from guarani November 18, 2020 10:09
@etoledom
Copy link
Contributor Author

etoledom commented Nov 18, 2020 •
edited
Loading

@guarani could you please take a look at the iOS side changes?
Also please take a look at the WPiOS related PR: wordpress-mobile/WordPress-iOS#15291

Thanks! 🙏

guarani
guarani approved these changes Nov 19, 2020
View reviewed changes
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.

Looking good from the iOS side (I didn't test Android), left some comments, but no blockers.

packages/react-native-editor/ios/GutenbergDemo/GutenbergViewController.swift Outdated Show resolved Hide resolved
packages/block-editor/src/components/index.native.js Outdated Show resolved Hide resolved
@etoledom etoledom force-pushed the rnmobile/file-block-III-wp-media-library branch from 17b0e96 to 657ac52 Compare November 19, 2020 21:36
@etoledom
Copy link
Contributor Author

Thank you for the review @guarani.
I have updated the PR with the requested changes.

I have also updated the Android native side (cc @jd-alexander )
These changes might break WP apps PRs, so we will need to update WPAndroid and WPiOS too with the ALL -> ANY change.

@jd-alexander jd-alexander changed the title [RNMoobile] Adding WPMediaLibrary source to File block [RNMobile] Adding WPMediaLibrary source to File block Nov 20, 2020
This was referenced Nov 24, 2020
Merged
Merged
@jd-alexander
Copy link
Contributor

Hi @etoledom I updated the media source picker's title and added an icon for the "Choose from device" option that displays an icon on Android.

Android iOS

Here, we have two variations of the button label for using a file from the device.

Should we be using.

  1. Choose from device
  2. Pick a file

I opted for "Choose from device" because it's used for the other blocks. Let me know what you think.

Even though this solution works I noticed that the "Choose from device" option for the File block is below the WP media option but for the other blocks, it's the first option. Below is a clear example.

Android iOS

This a minor thing. I looked at the code and it seems the internal sources eg. WP Media Library is appended to the other media options which we have provided hence the ordering. Should we modify this in any way?

@etoledom etoledom force-pushed the rnmobile/file-block-III-wp-media-library branch from d2f7401 to a932bfa Compare November 25, 2020 11:40
@etoledom
Copy link
Contributor Author

Here, we have two variations of the button label for using a file from the device.

We should use Choose from device as specified in the design file.
I have changed the title for the iOS Demo app.
WPiOS already shows Choose from device 👍

jd-alexander
jd-alexander approved these changes Nov 25, 2020
View reviewed changes
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.

The behavior works as expected based on the tests that were conducted on both platforms. LGTM 🚢

@etoledom etoledom merged commit b18fc28 into master Nov 25, 2020
@etoledom etoledom deleted the rnmobile/file-block-III-wp-media-library branch November 25, 2020 19:50
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guarani guarani guarani approved these changes

@jd-alexander jd-alexander jd-alexander approved these changes

@mchowning mchowning Awaiting requested review from mchowning

Assignees

@etoledom etoledom

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
Milestone
Gutenberg 9.5
Development

Successfully merging this pull request may close these issues.
3 participants
@etoledom @jd-alexander @guarani