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

JSDOC Property Descriptions #48

Closed
notoriousb1t opened this issue May 3, 2018 · 10 comments
Closed

JSDOC Property Descriptions #48

notoriousb1t opened this issue May 3, 2018 · 10 comments

Comments

@notoriousb1t
Copy link
Contributor

Having compatibility and initial values for each property in the jsdocs is great! One thing that would make these definitions stronger is if they had short descriptions for each. There are way too many css properties for any one developer/designer to memorize, so in TypeStyle we put a short little blurb about the property to give it some context:

  /**
   * Background-size specifies the size of a background image
   * @see https://developer.mozilla.org/en-US/docs/Web/CSS/background-size
   */
  backgroundSize?: 'auto' | 'cover' | 'contain' | CSSLength | CSSPercentage | CSSGlobalValues;

The short description lets the developer know that backgroundSize controls the background image.

Since the underlying types are from MDN's browser compat tables, I realize this information is not readily available from that datasource. I propose adding a json file to store the description or finding a datasource for it and then adding it to the output.

I can take a swing at creating a PR for it.

typestyle/typestyle#245

@frenic
Copy link
Owner

frenic commented May 5, 2018

There's no short description for properties available in the source as you said. They have been discussing it but I don't know how prioritized it is.

Until then, we could use the patching mechanism to complete property comments with short descriptions. It should be quite straight forward if there's a possibility to copy them from TypeStyle or somewhere else. You're welcome to give it a try.

We snap the @see link from mdn-browser-compat-data. They cover a majority of the properties, but some of them are missing. It would be great to patch those too if you're up for it.

@notoriousb1t
Copy link
Contributor Author

notoriousb1t commented May 6, 2018

Ok, so some early success is getting property descriptions. I wrote a simple crawler to pull the first meaningful paragraph from each @see url. Because the writing format on MDN is good and predictable, this turns out to be a good description for jsdocs. This approach allows us to request new info by emptying out the contents of that dictionary entry. This should also keep the build from spamming their site all the time.

Prototype: https://github.com/notoriousb1t/mdn-crawler

Here is the output of the process so far: https://github.com/notoriousb1t/mdn-crawler/blob/master/summaries.json

I am going to look for a way to fold this into the build process in csstype.

@frenic
Copy link
Owner

frenic commented May 7, 2018

This looks great! 👍 The less manual work the better. This even catched a 404 link for appearance that should be fixed or removed at their end.

It would be nice if it somehow could preserve simple text formatting like code and bold in markdown instead. Do you think that's manageable?

@notoriousb1t
Copy link
Contributor Author

I plugged in turndown (removing anchors) and that seems to work well. https://github.com/notoriousb1t/mdn-crawler/blob/master/summaries.json

I plan to start integrating this tonight

@frenic
Copy link
Owner

frenic commented May 8, 2018

I mentioned the patching mechanism before but that's for manual stuff so this should be its own thing. Thanks for looking into this.

@notoriousb1t
Copy link
Contributor Author

Some notes about the PR:

  • I couldn't figure out a way to fetch the summaries ahead of time for the properties that need it, so I ended up doing it on demand.
  • Fetching summaries on demand requests inline. I attempted to make this async, but it ended up polluting the code base with async/await. Instead I brought in sync-request which is probably okay since this is a build process and not code running directly in production. Maybe there is a different way to solve this problem.
  • I had to re-arrange the comment section a little bit so the summary would come first (as is typical in jsdocs)
  • urls.json has the cached results and should be checked in with the project so we can avoid hitting MDN each time we need to do a build.

Thoughts?

@frenic
Copy link
Owner

frenic commented May 8, 2018

Yeah, the whole thing should have been async from the start. I only have my self to blame for that. We should rewrite the whole thing one day.

It looks good overall. But one thought is that the summary is sneaked into __compat.mdn_summary as if it came from MDN source. What's the reason for that? I'm thinking that it could confuse someone else in the future.

@notoriousb1t
Copy link
Contributor Author

The primary reason I added it onto the data structure is that there are talks about adding into the json structure in the source on the repo. It should be pretty easy to transition if/when that happens. It is technically from an MDN source (although not from compatibility). I also wanted to avoid making unnecessary changes to the flow of the program.

Maybe we need to call it out as being scraped. Something like summary_from_mdn_web or move it to the top of the structure?

@frenic
Copy link
Owner

frenic commented May 8, 2018

Renaming it and only add the property to the return type instead of adding it to the source type would be enough for me. Does that sound reasonable?

@notoriousb1t
Copy link
Contributor Author

Sounds good to me. I will update the PR tonight with those changes.

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

No branches or pull requests

2 participants