Skip to content

Issue/1851 insert captions#2156

Merged
SergioEstevao merged 12 commits intodevelopfrom
issue/1851_insert_captions
Apr 20, 2020
Merged

Issue/1851 insert captions#2156
SergioEstevao merged 12 commits intodevelopfrom
issue/1851_insert_captions

Conversation

@SergioEstevao
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

SergioEstevao commented Apr 16, 2020

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
Copy Markdown
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
Copy Markdown
Contributor

@maxme maxme Apr 17, 2020

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
Copy Markdown
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

@designsimply designsimply Awaiting requested review from designsimply

@marecar3 marecar3 Awaiting requested review from marecar3

3 more reviewers

@hypest hypest hypest left review comments

@ceyhun ceyhun ceyhun approved these changes

@maxme maxme maxme approved these changes

Reviewers whose approvals may not affect merge requirements

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