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.

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

[RNMobile] Update Video caption placeholder color to match other placeholder text styles #16716

Conversation

@maxme
Copy link
Contributor

commented Jul 23, 2019

Fixes wordpress-mobile/gutenberg-mobile#996

Description

Update Video caption placeholder color to match other placeholder text styles.

Note: I had a look at the image caption and it's not using the same component (RichText for images, TextInput for videos), I wonder if this must be updated in the video block. Also, I guess this part could be factorized.

How has this been tested?

Manually tested via https://github.com/wordpress-mobile/gutenberg-mobile

Screenshots

out

Types of changes

Style 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.

cc @hypest (I can't add you as a reviewer to that PR, I don't have permissions).

@maxme maxme changed the title Update Video caption placeholder color to match other placeholder text styles [RNMobile] Update Video caption placeholder color to match other placeholder text styles Jul 24, 2019

@hypest

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

👋 @mchowning , can you add a review on this one? You recently worked on the caption for the Image block so this feels relevant.

@hypest hypest requested a review from mchowning Jul 24, 2019

@mchowning
Copy link
Contributor

left a comment

LGTM! 🚀

I had a look at the image caption and it's not using the same component (RichText for images, TextInput for videos), I wonder if this must be updated in the video block.

I think you're right and I didn't see any issues already tracking this, so I went ahead and opened wordpress-mobile/gutenberg-mobile#1244.

@maxme I'm not sure if you have the needed permissions to merge this yet. Just let me know if you don't and you'd like me to take care of that.

@maxme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I think you're right and I didn't see any issues already tracking this, so I went ahead and opened wordpress-mobile/gutenberg-mobile#1244.

Perfect!

@maxme I'm not sure if you have the needed permissions to merge this yet. Just let me know if you don't and you'd like me to take care of that.

I don't, merge it for me please 🍾 🎉

@mchowning mchowning merged commit 8f542ba into WordPress:master Jul 25, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.2 milestone Jul 25, 2019

@maxme maxme deleted the maxme:rnmobile/update-video-caption-placeholder-style branch Jul 25, 2019

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.