Skip to content

Youtube player - #619

Open
bernardomjunior wants to merge 9 commits into
developfrom
youtube-player
Open

bernardomjunior wants to merge 9 commits into
developfrom
youtube-player

Conversation

@bernardomjunior

Copy link
Copy Markdown
Collaborator

youtube videos now can be watched without leaving the application

Needed to call youtube API
…ation

Youtube's view is located at the same place as PhotoView's one, activity has to hide one or another depending of the situation
val extras = Bundle()
extras.putString(PinchToZoomActivity.ID, video.video_id)
extras.putString(PinchToZoomActivity.TYPE, PinchToZoomActivity.VIDEO)
startPinchActivity(item.context as AppCompatActivity, extras)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

É interessante pensar aqui no safe cast.
Se o contexto não for de uma AppCompatActivity esse cast vai lancar uma exception. Tudo bem que especificamente neste contexto dificilmente isso iria acontecer. Mas não custa previnir

Da uma sacada https://kotlinlang.org/docs/reference/typecasts.html#safe-nullable-cast-operator

Uma abordagem segura é usar o safe cast e se der sucesso chamar a activity usando a extension como vc fez ou se der nulo usar uma extension de context sem animação por exemplo. Sacou? Só para previnir mesmo

val imageId = intent.getStringExtra(ID)
imageId?.let {
photoView.loadImageUrl(
Utils.assembleGameImageUrl(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Se vc você refatorar, como você acha que seria uma forma mais elegante de fazer esse assembleGameImageUrl ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Prefiro extension em relação à utils. é essa a pergunta?

}

private fun loadVideo() {
photoView.visible(false)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Aqui é uma opinião pessoal.

Hoje eu prefiro criar extensions para cada ação. Exemplo:

visible()
invisible()
gone()

e uma que aceita um booleando para casos onde da para usar.
visibleOrGone(result)
visibleOrInvisible(result)

Por que isso?
Porque fica fácil de ler o código e entender o que está acontecendo e a chance de errar é menor também. Sacou?

Mas é uma questão bem pessoal

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

O que tu achas de view.visibility = View.Gone?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Então. É muito pessoal isso. Particularmente eu prefiro as coisas mais diretas, onde qualquer pessoa bate o olho e já sabe o que está acontecendo e não tem que pensar.
Seu eu olhar para um button.visible() ou para o button.gone() eu já sei o que vai acontecer com eles. Seu olho para um button.visibleOrGone(result) eu já sei o comportamento dele vai depender o result. Saca?

Comment thread .gitignore
app:layout_constraintRight_toRightOf="parent"
app:layout_constraintTop_toTopOf="parent"/>
android:id="@+id/btClose"
android:layout_width="@dimen/dp_25"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hoje eu não usaria esse tipo de dimen. Tem dois pontos aqui.
1 - Não é recomendado o uso de tamanhos impares
2 - Não tem diferença colocar 25dp hardcoded ou @dimen/dp_25

A melhor abordagem nesse caso seria a criação de padrões. Exemplo:

default = 8dp
medium = 16dp
large = 32dp

os nomes são apenas sugestões, vai depender do alinhamento dom designer e dev team.
A vantagem disso é que se no futuro o meu default não for mais 8dp eu simplesmente vou mudar o valor dele no dimens e refletir em todo o app. Agora imagina se o preciso dizer que onde tem dp_25 agora deve ter do_30. Ficaria muito ruim mudar o valor do dp_35 para 30 né. Ou muito trabalhoso ter que ficar trocando em todo o código. Sacou??

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

saquei, então devo rever o código feito antes e fazer alterações que o melhore, certo?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Não, por enquanto não. Podemos pensar mais para a frente em refatorar tudo para um módulo de design por exemplo. Mas por hora usa assim mesmo. Só comentei para ficar no seu radar.

@malkes malkes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No geral ficou tudo muito bom. O Kotlin está bem dentro do Code Style mesmo.
Fiz alguns comentários pontuais e comentei algumas coisas que já estavam no projeto mas que hoje eu já faria bem diferente. Da uma olhada e qualquer coisa a gente bate um papo depois para trocar uma ideia e refletir sobre os pontos.

@bernardomjunior
bernardomjunior requested a review from malkes August 17, 2020 18:48
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