-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Prettify code blocks in JavaScript docs #9565
Conversation
toString: function () { | ||
return "I'm an object!"; | ||
}, | ||
}; |
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.
One thing is that it destroys nicely aligned comments, which is a bit sad.
slice(start, end) | ||
slice(); | ||
slice(start); | ||
slice(start, end); |
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.
It also adds semicolons to Syntax section, which I'm not sure if we like. This section is odd though because although we are marking it up as JS, it isn't really.
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.
We don't like - i.e. omission is deliberate decision.
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.
Again if we want to run Prettier over JS, we should not mark up "syntax" as JS, because it isn't.
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.
Slightly amusingly to me, I just tagged you on a discussion on prettier here: https://github.com/mdn/content/discussions/9901
If we adopt prettier as a "default" then I'd rather mark up as JS and include the semicolon. The syntax highlighting makes this more readable.
type: 'Invertebrates', // Default value of properties | ||
displayType: function() { // Method which will display type of Animal | ||
type: "Invertebrates", // Default value of properties | ||
displayType: function () { |
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.
spot checking, and this is the only 'concern' ... but it's ok here, though may not always be - to have the 'this comment explains this function' that was on the end of the line coming after the thing it explains instead of before it.
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.
Also FWIW I much prefer function() {
over function () {
. If we did that there should be a fair bit of churn because it contradicts the style guide: https://developer.mozilla.org/en-US/docs/MDN/Guidelines/Code_guidelines/JavaScript#use_expanded_syntax
total.increase(); // #count now = 8 | ||
total.increase(5); // #count now = 13 | ||
console.log(total.current); // expected output: 13 | ||
total.reset(); // #count now = 7 |
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.
the spacing of having all the output lined up was intentional and has been removed. Again, this is ok
@@ -1,12 +1,13 @@ | |||
--- | |||
title: 'SyntaxError: missing } after property list' |
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.
All the 'don't do this' errors weren't corrected in this series. Not sure how the autoprefix missed it, but it's good it did.
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.
never mind. found a case where it corrected a 'don't do this'
function* f() {}; | ||
var obj = new f; | ||
function* f() {} | ||
var obj = new f(); |
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 should not have been changed.
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.
If we want to run Prettier on code samples, I think we should handle this by using a different info string for "bad JS" code samples, so Prettier doesn't think it's JS.
Here's a link to a Prettier playground showing some options:
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.
Can't it be trained to ignore js with the trailing example-bad
- i.e. js example-bad
is not treated as JS.
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.
Can't it be trained to ignore js with the trailing
example-bad
- i.e.js example-bad
is not treated as JS.
No. I think our only options would be to use the // prettier-ignore
comment, or not to use js
in the info string. I had hoped that we could use the <!-- prettier-ignore -->
before the code block, but as you can see from the huge link above that doesn't work.
We could train the syntax highlighter to recognize bad-js
as something to be highlighted (or even rewrite to as brush: js
in the markdown-to-html conversion).
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.
Or perhaps consider a more configurable alternative to prettier.
files/en-us/web/javascript/reference/global_objects/date/setutcminutes/index.md
Show resolved
Hide resolved
Honestly, lot of churn for little real value from what I'm seeing. |
I think we absolutely do want to do this, if we can configure it in a way we can all agree on - ideally aligned with other big projects. The starting point for those rules might be https://developer.mozilla.org/en-US/docs/MDN/Guidelines/Code_guidelines/JavaScript#use_expanded_syntax |
Prettier, by design, doesn't give you many configuration options. The idea is that instead of spending time arguing about how you want things to be formatted, you just use Prettier: https://prettier.io/docs/en/why-prettier.html. I'm very much in favour of this. Although, like everyone else, I have opinions about how code should be formatted, I'd much rather just adopt a consistent style, have tooling to make sure it gets applied, and never have to think about it again. |
I'm closing this, because although we still definitely want to do it, I don't expect this PR to be merged in anything like this form. |
This is the result of running:
...which has the effect of prettifying the JS code blocks in the JS documentation.
Just an experiment to see if we might want to do this.