-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Major ApolloServer API reference edits for new reference proof of concept #4494
Conversation
c6b8ade
to
fbac661
Compare
3daca85
to
8f9be58
Compare
@abernix This is ready for content review and merge-if-correct on my end if it is on yours |
46f8860
to
089bd3c
Compare
I think it might look nice to merge the name and type into one column with two lines, to give more horizontal space to the description. In general I think the table makes it a lot easier to browse the page! |
089bd3c
to
22ee3eb
Compare
Now awaiting some small style changes to the docs theme to support a two-column field table per comment above |
|
||
* `tracing`, `cacheControl`: <`Boolean`> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you lost these?
Personally I don't mind if we pretend tracing
doesn't exist. I'm pretty sure people do need to set the cacheControl options like defaultMaxAge
pretty frequently though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge improvement, both in terms of the improved phrasing of all too many things to mention and in terms of the visual treatment.
The only thing I had worth pointing out was that there was still some horizontal scrolling — even on Desktop viewports — happening due to the width of the Engine Reporting Options section, but @glasser already noted that and you've indicated some theme changes are forthcoming.
As another point worth mentioning, but of which I'm not asking for a solution, I find the overall syntax for building this page a bit hard to grok and I worry that it may be hard for contributors to be comfortable with. This is in a large part due to, e.g., ###
headings within <td>
tags and the tag-based nature of HTML (i.e., needing to track tags along with markdown at the same time).
I don't have any concrete suggestions on how to fix that right now, but since we already have YAML front-matter, perhaps a combination of markdown and YAML is another way the documentation could be defined? For example, a YAML structure which defines that we are an API-table-based dealie (and automatically applying the various heading/table treatments, defining the data declaratively in that format, while still supporting markdown within it.
This seems like a proprietary format that I'm describing, but maybe something already exists?
Anyhow, just some thoughts, but thanks so much for applying this! I look forward to seeing the theme adjustments prior to merging, but for now this is a tentative ✅ LGTM.
FWIW I agree that the formatting is a big pain, but also that the readability improvements are worth it. Certainly if there's something else with a nice build step to make it more maintainable that would be cool too! |
22ee3eb
to
b6ac016
Compare
@abernix Thanks for reviewing, and for your feedback on the formatting! I agree that the structure of this table is currently more complex than it should be, and we should find a way of hiding that complexity from authors. With front-end priorities elsewhere at the moment, I think we should proceed with this for now, and subsequent transformations will hopefully be comparatively lightweight. The new two-column formatting can now be seen here: https://deploy-preview-4494--apollo-server-docs.netlify.app/docs/apollo-server/api/apollo-server/ To the issue with horizontal scroll, I've resolved that for now by simply reverting the EngineReportingOptions table to the previous list format, since it's going away in a few days anyway. Once this is merged as a proof of concept, I'll start applying the same treatment to the balance of the API reference over the next week or two. |
It appears the branch that introduced this documentation was forked from prior to that change and that this particular changedidn't get pulled back in during conflict resolution. Original: 090586c#diff-c7c64f07d21dfa37133d976476500b03R65
It appears the branch that introduced this documentation was forked from prior to that change and that this particular change didn't get pulled back in during conflict resolution. Original: ea8c356
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing. I've applied two extra commits that appear to have gone astray and with them back in their rightful home in this beautiful new API treatment, I think we're good to go. ✅
Thank you so much for your work on this!
No description provided.