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

Issue/1851 insert captions #2156

Merged
merged 12 commits into from
Apr 20, 2020
Merged

Issue/1851 insert captions #2156

merged 12 commits into from
Apr 20, 2020

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented Apr 15, 2020 •
edited
Loading

Fixes #1851

This PR adds the caption info to the Media object that is sent between the bridge and the JS code.

I also use this opportunity to add Stub image inserters for the Media Library in Android demo app to allow to test this functionality.

Gutenberg PR: WordPress/gutenberg#21611
WP-iOS PR: wordpress-mobile/WordPress-iOS#13903
WP-Android PR: wordpress-mobile/WordPress-Android#11670

To test:

  • Start the demo app
  • Add an image block
  • Check that the predefined caption shows up for the Image

PR submission checklist:

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

@SergioEstevao SergioEstevao added the [Type] Bug Something isn't working label Apr 15, 2020
@SergioEstevao SergioEstevao added this to the 1.27 milestone Apr 15, 2020
@SergioEstevao SergioEstevao mentioned this pull request Apr 15, 2020
Merged
6 tasks
@SergioEstevao SergioEstevao requested a review from maxme April 16, 2020 14:58
ceyhun
ceyhun approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@ceyhun ceyhun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed and tested the iOS parts and it looks good 👍 The video caption is not inserted but I guess it's not required in this case.

@SergioEstevao
Copy link
Contributor Author

The video caption is not inserted but I guess it's not required in this case.

I actually checked on the web GB , and it's also not inserted there.

maxme
maxme approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

Tested on Android, LGTM. Thanks for adding the WP samples for testing. I'll check the wpandroid PR.

android/app/src/main/java/com/gutenberg/Media.java

import java.util.Locale;

public class Media implements RNMedia {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we had the kotlin dependency setup to reduce this to 4 LoCs :)

Copy link
Contributor

@hypest hypest Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Transformed the wish to a ticket 🤷‍♂️ #2203

@SergioEstevao SergioEstevao merged commit 6d7a995 into develop Apr 20, 2020
@SergioEstevao SergioEstevao deleted the issue/1851_insert_captions branch April 20, 2020 17:07
This was referenced Apr 27, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hypest hypest hypest left review comments

@ceyhun ceyhun ceyhun approved these changes

@maxme maxme maxme approved these changes

@designsimply designsimply Awaiting requested review from designsimply

@marecar3 marecar3 Awaiting requested review from marecar3

Assignees
No one assigned
Labels
[Type] Bug Something isn't working
Projects
None yet
Milestone
1.27
Development

Successfully merging this pull request may close these issues.

Images inserted from Media Library do not include captions previously saved in the Media Library
4 participants
@SergioEstevao @maxme @hypest @ceyhun