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

GraphQL - Embeds #898

Open
wants to merge 10 commits into
base: the-future
Choose a base branch
from
Open

GraphQL - Embeds #898

wants to merge 10 commits into from

Conversation

toyhammered
Copy link
Member

What

Why

Checklist

  • All files pass Rubocop
  • Any complex logic is commented
  • Any new systems have thorough documentation
  • Any user-facing changes are behind a feature flag (or: explain why they can't be)
  • All the tests pass
  • Tests have been added to cover the new code

@toyhammered toyhammered self-assigned this Nov 27, 2020
field :site_name, String,
description: SITE_NAME_DESCRIPTION,
null: false
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

app/graphql/concerns/optional_embed_description.rb Outdated Show resolved Hide resolved
app/graphql/types/interface/required_embed.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
class Types::Union::EmbedItem < Types::Union::Base
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good argument to using a union vs just referencing the base embed interface that all embeds inherit from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually sure. I've always preferred using a union vs referencing the base embed, I think it reads nicer. But both function the same way.

Copy link
Member Author

Choose a reason for hiding this comment

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

After reading this: https://artsy.github.io/blog/2019/01/14/graphql-union-vs-interface/ I think I prefer Unions.


field :url, String,
null: false,
description: 'The canonical URL of your object that will be used as its permanent ID in the graph'
Copy link

Choose a reason for hiding this comment

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

Line is too long. [102/100] (https://rubystyle.guide#80-character-limits)

value 'MALE', value: 'male'
value 'FEMALE', value: 'female'
value 'OTHER', value: 'other'
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

field :gender, Types::Enum::Gender,
null: true,
description: 'Their gender.'
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

implements Types::Interface::BaseEmbed

description 'Audio Properties'
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

Comment on lines +39 to +57
field :determiner, String,
null: true,
description: <<~DESCRIPTION.squish
The word that appears before this object's title in a sentence.
An enum of (a, an, the, "", auto). If auto is chosen,
the consumer of your data should chose between "a" or "an". Default is "" (blank).
DESCRIPTION

field :locale, String,
null: true,
description: <<~DESCRIPTION.squish
The locale these tags are marked up in.
Of the format language_territory.
Default is en_us.
DESCRIPTION

field :locale_alternative, [String],
null: true,
description: 'An array of other locales this page is available in.'
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we use any of these

Copy link
Member Author

Choose a reason for hiding this comment

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

should we not start bringing them in? They are optional.

@@ -0,0 +1,5 @@
class Types::Embed::AudioTagEmbed < Types::BaseObject
Copy link
Member

Choose a reason for hiding this comment

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

Why the Tag?

Copy link
Member Author

Choose a reason for hiding this comment

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

seemed kind of strange just being AudioEmbed and on the docs it used tag in the description. i.e: The og:video tag has the identical tags which I kinda liked. I can rename though.

class Types::Embed::VideoEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed

end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

@@ -0,0 +1,4 @@
class Types::Embed::VideoEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed

Copy link

Choose a reason for hiding this comment

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

Extra empty line detected at class body end. (https://rubystyle.guide#empty-lines-around-bodies)

class Types::Embed::MusicEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed

end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

@@ -0,0 +1,4 @@
class Types::Embed::MusicEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed

Copy link

Choose a reason for hiding this comment

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

Extra empty line detected at class body end. (https://rubystyle.guide#empty-lines-around-bodies)

@@ -0,0 +1,3 @@
class Types::Embed::ImageEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

@@ -0,0 +1,3 @@
class Types::Embed::BookEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

@@ -0,0 +1,3 @@
class Types::Embed::WebsiteEmbed < Types::BaseObject
implements Types::Interface::BaseEmbed
end
Copy link

Choose a reason for hiding this comment

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

Final newline missing. (https://rubystyle.guide#newline-eof)

@codeclimate
Copy link

codeclimate bot commented Nov 29, 2020

Code Climate has analyzed commit 5e9c522 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 12

View more on Code Climate.

@NuckChorris NuckChorris added this to the V4: Feeds milestone Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants