Skip to content

Rich Image Caption feature branch#975

Merged
etoledom merged 20 commits intodevelopfrom
feature/caption
May 16, 2019
Merged

Rich Image Caption feature branch#975
etoledom merged 20 commits intodevelopfrom
feature/caption

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented May 10, 2019

This PR merges the Rich Image Caption feature branch.

All this code has been already tested and approved here: #941

A smoke test should be enough to ✅ this PR.
For more detailed test cases, please check out #941

gutenberg side PR: WordPress/gutenberg#15571

cc @mchowning

center_caption_resized

IMG_1668

@etoledom etoledom added the [Type] Enhancement Improves a current area of the editor label May 10, 2019
@etoledom etoledom added this to the v1.5 milestone May 10, 2019
@etoledom etoledom self-assigned this May 10, 2019
@etoledom etoledom marked this pull request as ready for review May 10, 2019 17:31
@etoledom etoledom requested a review from hypest May 10, 2019 17:31
@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented May 13, 2019

Updated this PR and WordPress/gutenberg#15571 to incorporate the changes from the video block.

I would appreciate a couple of 👀 from @mchowning and @marecar3 or @pinarol to be sure that everything is in order, since there were a few conflicts to be resolved on image/edit.native.js.

Thank you!

@marecar3
Copy link
Copy Markdown
Contributor

Hey, @etoledom @mchowning this PR looks good :)

However, there is some case where it crashes. So if you try to upload the video, it will crash with red screen.

The issue is in this line of code: https://github.com/WordPress/gutenberg/blob/449c25459c917b546cdfd786dbfd56dd3cb9a43a/packages/block-library/src/video/edit.native.js#L218

Having that on mind we probably want to remove ( styles[ 'caption-text' ].fontFamily ) and also import styles from '../image/styles.scss'; from video block

and maybe to move this :

.caption-text { font-family: $default-regular-font; }

into the corresponding video style file: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/video/style.native.scss

@etoledom
Copy link
Copy Markdown
Contributor Author

Nice catch @marecar3 !
I have added the missing style (fonts) to the video styles scss file.
It was removed from the Image block since it's not used by RichText.

A better "fix" will be when we use RichText Captions in Video Blocks too (in the future).
Would you mind checking it again?

Thank you!

@marecar3 marecar3 self-requested a review May 14, 2019 16:28
Copy link
Copy Markdown
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Nice work @etoledom! @mchowning! :)
LGTM!

@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented May 14, 2019

Thank you @marecar3 !!
I'll wait for #949 to be merged before merging this one. Just in case, to avoid possible troubles and more delays merging that fix.

cc @mchowning @hypest

@etoledom etoledom merged commit 2d448a3 into develop May 16, 2019
@etoledom
Copy link
Copy Markdown
Contributor Author

Thank you all, and congrats @mchowning !
Great job 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement Improves a current area of the editor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants