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

Show deprecation information in the Docs sidebar #157

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

davidcelis
Copy link
Contributor

The GraphQL specification states clearly:

Tools built using GraphQL introspection should respect deprecation by
discouraging deprecated use through information hiding or
developer‐facing warnings.

Currently, however, GraphiQL doesn't give any indication of whether or
not a field or enum value is deprecated. This patch rectifies this by
adding some red text next to field names and enum value names that just
says "(DEPRECATED)". If a deprecation reason is given for a field or
enum value, that is rendered (also in red) underneath the field or
value's description.

Closes #34.

Signed-off-by: David Celis davidcelis@github.com

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@davidcelis
Copy link
Contributor Author

Signed the CLA! A couple of notes about this patch also 😄

  1. I ended up re-using the Description component to render a deprecation reason. It seemed like another good use-case so I hope that's okay but, if not, let me know and I'm happy to make a new component.
  2. I didn't add any tests, but it seems like the markup isn't generally tested. I may have missed something, so if there are tests I should be adding, let me know!
  3. I'd also really love to visually alter requested fields in the query editor if they're deprecated. I looked through some of the CodeMirror stuff but I just couldn't figure out where field validation was happening 😓 If someone could point me in the right direction there, I can add another patch to this PR ✨

@ghost ghost added the CLA Signed label Aug 2, 2016
@ghost
Copy link

ghost commented Aug 3, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@asiandrummer
Copy link
Contributor

asiandrummer commented Aug 3, 2016

Hi @davidcelis - thanks for contributing!

I ended up re-using the Description component to render a deprecation reason.

I think it's fine, although I'd like Description component to be called differently as it's really just a component that can hold markdown texts I feel like. Either that or we could just create another component that even defaults the style to use alert text:

<DeprecatedReason markdown={field.deprecatedReason} />

I'm personally more attracted to the first approach of changing the component name.

I didn't add any tests, but it seems like the markup isn't generally tested.

My long-awaited goal... :p Would you be interested in becoming a pioneer and write tests for this?

I'd also really love to visually alter requested fields in the query editor if they're deprecated.

This would be a bit tricky since the styling in QueryEditor is determined by the style token passed in from online-parsing the document, which is defined in codemirror-graphql repo. When you mention "field validation", I'm guessing you meant how CodeMirror uses the parsed document/AST from codemirror-graphql online parser and figure out how to treat/style each token. It's pretty straightforward to modify token context passed in from the online parser and add a new style to reflect that new style context, but I'm in process of moving the language service portion as a separate repository - I can take care of this after that.

A workaround to make this happen would be creating a new style in codemirror.css and manually attach it to components' classNames prop to style deprecated fields/values for now, and remove this once the process becomes organic like described above. I'd be down for this approach since I'm pretty horrible with making the styling look good and would love to get some help from you :D

@ghost ghost added the CLA Signed label Aug 3, 2016
@@ -495,6 +495,8 @@ class TypeDoc extends React.Component {
{argsDef && [ '(', <span key="args">{argsDef}</span>, ')' ]}
{': '}
<TypeLink type={field.type} onClick={onClickType} />
{field.isDeprecated &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small nit: I'd like to keep the brackets open/close using its own line

{
  field.isDeprecated &&
  <span ... />
}

@davidcelis
Copy link
Contributor Author

@asiandrummer Thanks for the comments!

Just a small nit: I'd like to keep the brackets open/close using its own line

Done in 8cd42bc 😄

I'd like Description component to be called differently as it's really just a component that can hold markdown texts I feel like.

Agreed. I've renamed it to MarkdownContent but I can change it to something else too if that doesn't quite fit the bill.

I'm in process of moving the language service portion as a separate repository - I can take care of this after that.

A workaround to make this happen would be creating a new style in codemirror.css and manually attach it to components' classNames prop to style deprecated fields/values for now, and remove this once the process becomes organic like described above.

I can try out a workaround, but it sounds like we'll be able to take care of this in a better way once the language service portion is moved out. I'm happy to help with the visual alteration once that happens! I was thinking a strikethrough would be a good way to denote deprecation.

Would you be interested in becoming a pioneer and write tests for this?

I'm happy to lay some foundations for this, though I should probably also say that this is my first contribution to a React application. So that might take some extra time to learn how to do this 😄

@asiandrummer
Copy link
Contributor

Love the changes - would you mind squashing commits into a single one if possible?
Unless you'd like to tackle the above couple of things (tests/query editor visual changes), I'm ready to merge it after commit squashing!

The GraphQL specification [states clearly](https://facebook.github.io/graphql/#sec-Deprecation):

> Tools built using GraphQL introspection should respect deprecation by
> discouraging deprecated use through information hiding or
> developer‐facing warnings.

Currently, however, GraphiQL doesn't give any indication of whether or
not a field or enum value is deprecated. This patch rectifies this by
adding some red text next to field names and enum value names that just
says "(DEPRECATED)". If a deprecation reason is given for a field or
enum value, that is rendered (also in red) underneath the field or
value's description.

Closes graphql#34.

Signed-off-by: David Celis <me@davidcel.is>
@davidcelis
Copy link
Contributor Author

Squashed!

@asiandrummer asiandrummer merged commit ee44d44 into graphql:master Aug 3, 2016
@davidcelis davidcelis deleted the deprecation-nation branch August 3, 2016 21:18
@Globegitter
Copy link

@davidcelis how exactly is this supposed to look like? I just upgraded and added a deprecated field. I am seeing the deprecation text in red when I click on the field but I am not seeing anything in the overview, where I have the list of field. Is that as expected?

acao pushed a commit to acao/graphiql that referenced this pull request Jun 1, 2019
…transform-es2015-object-super-6.24.1

Update babel-plugin-transform-es2015-object-super to the latest version 🚀
acao pushed a commit to acao/graphiql that referenced this pull request Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants