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

doc: make pull-request guide default branch agnostic #41299

Merged
merged 3 commits into from
Dec 31, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 23, 2021

Refs: #33864

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 23, 2021
$ cd node
$ git remote add upstream https://github.com/nodejs/node.git
$ git fetch upstream
```sh
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but this change results in wonky highlighting that makes no sense to me.

image

This is because sh makes it think it's syntax-highlighting a bash script rather than rendering shell CLI stuff. I think txt is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think text make no sense semantically speaking. I don't feel strongly about syntax highlighting though, maybe we should disable syntax highlighting for (ba)sh altogether? From a quick glance it looks like it's never useful (and sometimes it even adds confusion as this one). If that sounds good to you, I can open a PR doing that.

Copy link
Member

Choose a reason for hiding this comment

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

If that sounds good to you, I can open a PR doing that.

Is it possible to do that? This is a document that gets displayed in the GitHub interface, not on our web pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, we don't have control over this indeed, I thought you meant the HTML version of the docs

Copy link
Member

Choose a reason for hiding this comment

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

A valid flag in Linguist (which is what the GitHub interface uses) is fundamental. It's an alias for text but maybe it is more semantically tolerable?

Another possibility is to adjust our rules to omit use of a flag when it makes sense.

And one more possibility would be to not use code blocks for this sort of thing. Like, in HTML, these kind of code blocks might be a <code> elements but maybe these kind of "here's a list of commands to run" should really be <pre> elements or something else?

Copy link
Member

@Trott Trott Dec 25, 2021

Choose a reason for hiding this comment

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

And of course the two status quo options:

  1. Don't worry about the wonky highlighting.
  2. Use text and either convince ourselves that it makes sense semantically or don't worry that maybe it doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I had unlimited time and motivation, I would track down which library GitHub is using for syntax highlighting and beg them to get rid of this keyword highlighting which is more confusing than helping. But since I don’t: I think, like the bash vs sh discussion we had, since the semantic is not really a user facing feature, it should not matter and we should use whatever works with the current config (so text). I would still be very interested to know if the semantic part resonates with others, in particular Id be interested to know if that makes any difference to folks using a screen reader to collaborate.

Copy link
Member

@Trott Trott Dec 25, 2021

Choose a reason for hiding this comment

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

If I had unlimited time and motivation, I would track down which library GitHub is using for syntax highlighting and beg them to get rid of this keyword highlighting which is more confusing than helping.

GitHub uses https://github.com/github/linguist.

Valid flags are definied in https://github.com/github/linguist/blob/master/lib/linguist/languages.yml.

They say they use Linguist for detection but it's not clear to me that it's used for actual highlighting.

I would still be very interested to know if the semantic part resonates with others, in particular Id be interested to know if that makes any difference to folks using a screen reader to collaborate.

Since screen readers use the HTML output in the GitHub interface, and the language flags end up as CSS, class attributes, and things like that, I doubt they play a semantic role for many from an a11y perspective. (But the weird highlighting might.) The exception would be someone reading raw markdown (whether with assistive technology or not).

Co-authored-by: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 26, 2021
@aduh95 aduh95 merged commit c5ac0c6 into nodejs:master Dec 31, 2021
@aduh95 aduh95 deleted the pr-guide-main branch December 31, 2021 00:07
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 31, 2021

Landed in c5ac0c6

targos pushed a commit that referenced this pull request Jan 14, 2022
Refs: #33864

PR-URL: #41299
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Refs: #33864

PR-URL: #41299
Reviewed-By: Rich Trott <rtrott@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Refs: nodejs#33864

PR-URL: nodejs#41299
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Refs: #33864

PR-URL: #41299
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants