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

Add better documentation endpoints #248

Merged
merged 13 commits into from
Sep 28, 2020
Merged

Add better documentation endpoints #248

merged 13 commits into from
Sep 28, 2020

Conversation

ashley-hebler
Copy link
Member

@ashley-hebler ashley-hebler commented Sep 14, 2020

What's this PR do?

Converts the style-doc logic to use typescript which helps breaks down various parts of the style system (e.g. colors, modifiers, tokens) into different types.

Classes added (if any)

None

Classes removed (if any)

None

Why are we doing this? How does it help us?

TBH, this is probably complete overkill. I've been wanting to clean up that SCSS comment parsing step, which is what builds out the CSS data for the docs and this just kind of helped me bring order to that chaos.

@AndrewGibson27, there's a ton of code here and I don't want you to spend too much time sorting through it. The style-doc.ts part is the only real change and it only pertains to the docs — nothing that affects where queso styles are used. I'm sure I broke all the typescript rules along the way.

FWIW npm run dev is faster with the cleaner, more structured style data.

How should this be manually tested?

npm install
npm run dev

Just confirm the docs are loading properly and you don't run into any build errors.
There's a little blurb about the endpoints at the bottom of the about page, which should help sort through the new data output: http://localhost:8080/about/

Does this introduce a breaking change where queso-ui is used in the wild? If so, is there a relevant branch/PR to accompany this release?

Doesn't even merit a release

@ashley-hebler
Copy link
Member Author

@AndrewGibson27 ps I just realized I wasn't linting typescript so working on those fixes now.

@AndrewGibson27
Copy link
Contributor

AndrewGibson27 commented Sep 17, 2020

I love this. TypeScript is wonderful in general, but I think your choice to use it on a documentation site -- where by definition things need to be broken into very granular types -- is spot on.

You might well know or have thought about these things. Feel free to ignore if so.

  • I noticed you added some types for the KSS library. Looks like there are official typings for that lib, which you could install and then drop the custom ones. In fact, I bet there are official typings for many of the libraries you're using, as well as Node typings that give you those nice annotations for built-in stuff. Maybe adding those in this PR would be overkill, or maybe you don't care as much about getting typings for 3rd-party code. Just thought I'd mention.
  • Using CommonJS modules in .ts files (like here) is a slight anti-pattern, given CJS modules are dynamic by nature, and TS is dependent on static typing. But again, maybe getting type annotations for everything isn't a priority or even necessary.
  • This is just a personal opinion, but I'm a fan of explicitly annotating function return types. TS can usually infer that, but I think it's helpful inline documentation.
  • Lastly, if you pulled the TS + ESLint config from either texastribune or dot, I apologize for how confusing it is. And I'm all ears if you have ways to simplify it. The ESLint ecosystem is such a wild west of plugins, parsers, presets and rules. I have trouble understanding it all.

@ashley-hebler
Copy link
Member Author

Hey @AndrewGibson27, I finally had some time to get back to this. I appreciate your helpful tips.

  • Those KSS official typings were much better than manually entering those. I remember I attempted to use them at first, but got a little lost. After spending more time with them though, I figured out how to get them referencing those instead so I was happy to delete all those lines.
  • I switched my utils over to an import statement, but left some of the other libs as CJS. That's good to know about that antipattern in TS. I will def keep that in mind in the future and start relying less on CJS in general. I was reading that there could be some perf benefit in being able to explicit take parts of a package. That hadn't occurred to me before! And as we update node looks like we'll get that ES module support.
  • Good call! I noticed Ty doing that in some of our TT python recently. It is helpful for sure. I added it in the .ts file and will also keep that practice in mind in the future.
  • Oh yeah for the linting, I just kinda winged it. I don't think it's set up quite right for catching TS syntax, but eh we'll get there eventually ha. I agree, the ecosystem there is confusing.

I'm going to go ahead and merge this just to get the new github usage data up. It now has all of the new data viz instances! Thanks again for this review. I learned a lot from it!

@ashley-hebler ashley-hebler merged commit 15a1320 into main Sep 28, 2020
@ashley-hebler ashley-hebler deleted the new-endpoint-2 branch November 6, 2020 21:24
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