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

Benpa/spec tables prettier #1599

Merged
merged 8 commits into from
Aug 31, 2018
Merged

Benpa/spec tables prettier #1599

merged 8 commits into from
Aug 31, 2018

Conversation

benparsons
Copy link
Member

This change does two things:

  • makes all the tables aligned the same
  • moves the unclear titles of types to the caption of the correct tables

@benparsons benparsons requested a review from a team August 29, 2018 15:42
@turt2live
Copy link
Member

Fixes #1508

As a general comment, the headings feel a bit big. How do you feel about knocking it down in size a bit? (and maybe keeping the left align? I found it a bit hard to find the heading, surprisingly)

@benparsons
Copy link
Member Author

I played with it a little and was stuck on those points exactly. Happy to adjust

@turt2live
Copy link
Member

It looks good to me, although I think this is one for @ara4n to review (given it's stylistic).

Also, the index sprouted an extra title:
image

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Not that I were happy with column headers like "Identity Server Information Parameter" (see the freshly merged .well-known section), especially since the very same name is already there just before the table.
There's also an extraneous blank line between "Request/Response format" and the actual table for it.

@ara4n
Copy link
Member

ara4n commented Aug 29, 2018

@benparsons many thanks for hacking on this :)

There is definitely still room for more cosmetic improvement here (perhaps ask @nadonomy for an aesthetic consultation?) in terms of consistent heading sizings and sufficient vertical whitespace above & below headings to ensure good section breaks between them and general clarity over what heading goes with what.

I think we might also need some padding in tables in general. A particularly bad example is the main index page of the specs themselves, which for whatever reason just doesn't have enough visual weight to draw the eye to the entries or steer the user to realising that this is the front door of the actual spec that they need to click on to proceed meaningfully:

screen shot 2018-08-29 at 23 56 37

Perhaps some of this is because the APIs simply don't feature in the TOC for the spec itself - they're just hidden in section 1 in what feels like it might be just an intro para?

In terms of the table changes - it's definitely a massive improvement that the columns are aligned, however making them 100% wide (especially on a typical fullscreened browser) makes the columns quite spread out and the information quite hard to correlate horizontally:

screen shot 2018-08-30 at 00 01 12

Much like newspaper columns, i think we might be best off imposing an arbitrary maxwidth on them - say 800px or so, and yes, that means rows will wrap, but the legibility overall might be better. Some gentle zebra striping might help too.

The headings are also great to be in the right place, but definitely need a bit of lipstick from @nadonomy (getting the left margin right; using a proportional font (or being consistent with a monospace font; using a prettier font than Courier - perhaps Consolas or something); fixing the font size & vertical margins to make it more clearly a header; etc.

Overall it's a massive improvement though - thank you! It would be super-useful to stare at the spec in general a bit from a legibility and coherency perspective and see what other bits are just mangled in the current form.

(Aside: in an ideal world i wish the spec looked something like https://github.com/lord/slate)

@turt2live
Copy link
Member

Aside: in an ideal world i wish the spec looked something like https://github.com/lord/slate

fwiw I played a little bit with a similar thing a while ago: matrix-org/matrix.org#197 / https://redoc.matrix.c2s.temp.t2host.io/ - overall it doesn't seem all that bad, despite our descriptions not making a whole lot of sense when rendered in this way.

@ara4n
Copy link
Member

ara4n commented Aug 29, 2018

ooooooh. that looks really nice - and almost certainly better than the default swagger viewer we have for the API (even if it doesn't replace the spec itself?) But yes, the descriptions leave a bit to be desired and i wonder what other things it drops on the floor (things like NOTE and WARNING boxes perhaps?). Plus it really feels like we should (eventually) just have one spec doc that covers both the API reference and the spec itself, albeit nicely navigable in this sort of template.

@benparsons
Copy link
Member Author

re having the key name in the column header: is this universally disliked? Happy to remove if so.

I have only been looking at the spec through the lens of matrix.org/spec, which adds other CSS which may not be there when looking at these files directly. e.g. the table width being 100% is 100% of the container, which is 1080px. Similarly, the table layout and header spacing is already a lot better with matrix.org styles. I will try to get this consistent when looking at the raw files.

Slate looks really good. @nadonomy and I spoke in the recent past about transferring a lot of content from matrix.org into GitBook, which is a similar deal. (eg https://docs.duckduckhack.com/frontend-reference/setting-goodie-display.html)

It would be great to have everything under /docs in the same format, especially if we can maintain the markdown used for /docs^/docs/spec

@richvdh
Copy link
Member

richvdh commented Aug 31, 2018

re having the key name in the column header: is this universally disliked? Happy to remove if so.

I'm afraid I don't really understand this question. What do you mean by the key name?

@benparsons
Copy link
Member Author

@richvdh see kitsune's comment, but n/m: it's already removed

@benparsons
Copy link
Member Author

Nested lists now working:

screen shot 2018-08-31 at 14 26 38

Headers for HTTP actions are like titles, captions on tables look better

screen shot 2018-08-31 at 14 29 56

@Half-Shot
Copy link
Contributor

So pretty 👀

@richvdh
Copy link
Member

richvdh commented Aug 31, 2018

Matthew says he's happy for this to be merged.

@richvdh richvdh merged commit 8773b93 into master Aug 31, 2018
@afranke afranke deleted the benpa/spec-tables-prettier branch December 6, 2021 12:44
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.

6 participants