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

Remove all except cards and endscreens #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove all except cards and endscreens #32

wants to merge 2 commits into from

Conversation

kangkyu
Copy link
Contributor

@kangkyu kangkyu commented Nov 25, 2019

closes #30

YouTube removed annotations (except cards and endscreens) on
January 15, 2019. Remove unnecessary code
@coveralls
Copy link

coveralls commented Nov 25, 2019

Coverage Status

Coverage decreased (-1.5%) to 97.541% when pulling 75b73ad on clean-up into 1c25f2c on master.

@kangkyu kangkyu requested a review from dgb November 25, 2019 20:35
Copy link

@dgb dgb left a comment

Choose a reason for hiding this comment

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

Comments are just on some small things. LGTM

@@ -56,34 +48,17 @@ def json_to_annotations(json)

def annotation_class(data)
case data['style']
Copy link

Choose a reason for hiding this comment

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

Can these nested conditionals be simplified now? Seems like it could just be

if data['style'] == 'branding'
  Annotations::Branding
else
  Annotations::Card
end

expect(annotations[3].link[:type]).to be :website
expect(annotations[3].link[:new_window]).to be true

expect(annotations[4]).to be_a Yt::Annotations::Branding
Copy link

Choose a reason for hiding this comment

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

This is a preexisting thing, but I'd prefer we tested this via behavior.

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.

clean up all but end screens and info cards
3 participants