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

Adding captionBackgroundView #94

Merged
merged 2 commits into from
Feb 16, 2018

Conversation

yhkaplan
Copy link
Collaborator

@yhkaplan yhkaplan commented Feb 9, 2018

目的 Purpose

キャプションを表示する機能はあるのですが、画面によっては見えにくくなったりしますので、
半透明なバックラウンドビューを見せたいです。
There's a feature for showing captions, but it's difficult to see depending on the screen,
so I would like to show a translucent background view.

実装内容

  1. キャプション背景の色選択
  2. キャプション背景の固定の高さオプション
  3. キャプション背景のAutoLayoutを使った、キャプションテキストに合わせるオプション

今後PRを考えている要素 Areas of consideration for future PRs

  1. キャプションテキストのカストマイズオプション(shadowなど)
    More customization options for caption text (shadow, etc)
  2. アニメーションの一部のリファクタ
    Some refactoring with the animations
  3. ドキュメンテーションの追加
    Some additional documentation

@nakajijapan
Copy link
Owner

nakajijapan commented Feb 14, 2018

@yhkaplan

Thank you for the great PR! I will review it today or tomorrow.
Please wait for a while until the review...

@yhkaplan
Copy link
Collaborator Author

@nakajijapan OK, thanks!

lazy var captionBackgroundView: UIView = {
var captionBackgroundView = UIView()
if let captionHeight = captionHeight {
var y = view.bounds.height - captionHeight - 32.0
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know what 32.0 is the value for.
Could you change the magic number to the variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the padding of 32 below the caption label: Line 324

I will make it a constant and put a comment above the constant declaration to make it more clear!

Copy link
Owner

Choose a reason for hiding this comment

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

oops.... ok

Copy link
Owner

@nakajijapan nakajijapan left a comment

Choose a reason for hiding this comment

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

Thanks for great PR~~~~~~~!
But I wan you to fix one point.

@nakajijapan nakajijapan self-requested a review February 16, 2018 08:32
@nakajijapan
Copy link
Owner

🆗

@nakajijapan nakajijapan merged commit 54df4d6 into nakajijapan:master Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants