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

Markdown linting and spell checking documentation #1002

Merged
merged 9 commits into from
May 31, 2023

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented May 24, 2023

Applies markdown linting to all documentation.

  • This is worth doing as it makes it a lot simpler to pick out new errors or broken mark-up when editing/maintaining the docs as well as making the source files themselves more readable.

Corrects a number of spelling errors and americanizes all spellings

  • we've been gradually sliding towards British English spellings, but the original core of the documentation is in 'merican.

Fixes multi-lang code block on supported platform page

  • This broke in the docusaurus 2 upgrade as the syntax for it changed

Customizes Markdown linting rules

  • Some things we're unlikely to change (e.g. customized headers, line length etc.) or relate to docusaurs' use of markdown (not aware of a customized set of linker rules for docusaurus, if there is one, lemme know!)

@kriswest kriswest added docs Documentation website labels May 24, 2023
@netlify
Copy link

netlify bot commented May 24, 2023

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit d0ef5f0
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6475e7cd9b2f7d00088e363d
😎 Deploy Preview https://deploy-preview-1002--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@kriswest kriswest added this to the 2.1 milestone May 24, 2023
@kriswest kriswest requested a review from a team May 25, 2023 09:02
@kriswest
Copy link
Contributor Author

@greyseer256 Could you also give this one a quick glance. Doesn't change content, only spelling, markdown and docusaurus errors. Once merged I can drop overlapping changes from some other PRs by rebasing them that I told distract reviewers.

rikoe
rikoe previously requested changes May 25, 2023
Copy link
Contributor

@rikoe rikoe left a comment

Choose a reason for hiding this comment

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

I don't agree with changing headers from H4 to simply being bold. As I mentioned on another PR, replacing the H4 with just bold text removes the hyperlinks.

e.g. I can currently navigate to https://fdc3.finos.org/docs/api/ref/DesktopAgent#examples, or send that link to someone.

This is the kind of opinionated change that linters demand that actually adds no value, and removes some. I don't see the point of this.

I have added more comments in-line, but I also have issues with forcibly enclosing hyperlinks in angle brackets, and forcibly changing all bullets to be stars (*).

docs/context/ref/Instrument.md Show resolved Hide resolved
docs/intents/ref/StartCall.md Outdated Show resolved Hide resolved
docs/app-directory/spec.md Outdated Show resolved Hide resolved
@kriswest
Copy link
Contributor Author

kriswest commented May 25, 2023

I don't agree with changing headers from H4 to simply being bold. As I mentioned on another PR, replacing the H4 with just bold text removes the hyperlinks.

h4 is being used improperly in most cases as its often out of order and creates duplicated headings, both of which are bad for accessibility as it makes navigation of the page by a user with a screen reader significantly more difficult (I'm told). Before changing these I confirmed that there were no internal links using any of these headings.

Further, I'd argue that there is always an adjacent heading that provides a more semantically useful link, for example the one you gave: https://fdc3.finos.org/docs/api/ref/DesktopAgent#examples is right underneath https://fdc3.finos.org/docs/api/ref/DesktopAgent#addcontextlistener

If you are adamant that we need direct links to the examples and I were I to walk this back I'd prefer that we using h3/h4/h5 as appropriate for the navigation hierarchy - noting that this will lead to some differences in formatting or we would also be accepting a less accessible format for the page (boo hiss).

@rikoe
Copy link
Contributor

rikoe commented May 25, 2023

@kriswest what do you mean by "used out of order"?

@kriswest
Copy link
Contributor Author

kriswest commented May 25, 2023

@kriswest what do you mean by "used out of order"?

sometimes we have an h4 appearing under an h1 or h2, in one case it was under an h5 (quite often, which is why the linter flags them). Hence, I thought it was just being used to pick a style in most cases (for a list header rather than a heading - the 'see also' usage was definitely a list header).

Note in the intent files they were h2s (under an h1), they render larger and appear in the navigation, hence those have been left alone. E.g.: https://fdc3.finos.org/docs/intents/ref/StartCall

@kriswest kriswest requested a review from rikoe May 25, 2023 11:11
Copy link
Contributor

@mattjamieson mattjamieson left a comment

Choose a reason for hiding this comment

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

Looks good to me. OK with the angle-bracketed URLs and consistency for bullets. I'll live with the American English 😁

No strong opinion on having "Examples" as a heading, happy either way.

One (very!) minor formatting change.

docs/context/ref/Email.md Outdated Show resolved Hide resolved
Co-authored-by: Matt Jamieson <10372+mattjamieson@users.noreply.github.com>
@kriswest kriswest requested a review from mattjamieson May 30, 2023 12:11
Copy link
Contributor

@mattjamieson mattjamieson left a comment

Choose a reason for hiding this comment

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

LGTM

@kriswest
Copy link
Contributor Author

kriswest commented May 31, 2023

@rikoe apparently branch protection won't less this be merged until you re-review or I dismiss your review (which just sounds rude so thought I'd ping you first).

Edit: at maintainers meeting we heard that Riko is on holiday and there was a consensus that this can now be merged.

@kriswest kriswest dismissed rikoe’s stale review May 31, 2023 12:12

Because he's on holiday

@kriswest kriswest merged commit ce5c13a into master May 31, 2023
@kriswest kriswest deleted the markdown-linting-api-spec branch May 31, 2023 13:11
@kriswest kriswest mentioned this pull request Jul 25, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants