-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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
fix: syntax errors in JS example sections (v2) #18191
fix: syntax errors in JS example sections (v2) #18191
Conversation
@@ -78,14 +78,14 @@ This renders a scene. But it's inefficient, because it allocates as local variab | |||
A simple change can optimize this significantly: | |||
|
|||
```js | |||
const vertexList = ... | |||
const vertexList = "..."; |
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 turning "obviously incorrect" code into "less obviously incorrect" code. This entire page seems incomplete—not sure what to do about 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.
const vertexList = "..."; | |
const vertexList = [/*...*/]; |
?
@@ -68,7 +68,7 @@ const SpeechRecognitionEvent = window.SpeechRecognitionEvent || webkitSpeechReco | |||
The next part of our code defines the grammar we want our app to recognize. The following variable is defined to hold our grammar: | |||
|
|||
```js | |||
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral' ... ]; | |||
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral' /* ... */ ]; |
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.
As trailing commas are allowed, might this fit better?
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral' /* ... */ ]; | |
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral', /* ... */ ]; |
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 (I don't have strong feelings):
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral' /* ... */ ]; | |
const colors = [ 'aqua' , 'azure' , 'beige', 'bisque', 'black', 'blue', 'brown', 'chocolate', 'coral' /* , ... */ ]; |
@@ -29,7 +29,7 @@ A `WebGLActiveInfo` object is returned by: | |||
- {{domxref("WebGLRenderingContext.getActiveUniform()")}} or | |||
- {{domxref("WebGL2RenderingContext.getTransformFeedbackVarying()")}} | |||
|
|||
```js | |||
```glsl |
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 sounds like WebIDL: let's remove the whole block, it doesn't bring anything.
```glsl | ||
texParameterf(target, GLenum pname, GLfloat param) | ||
texParameteri(target, GLenum pname, GLint param) |
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.
```glsl | |
texParameterf(target, GLenum pname, GLfloat param) | |
texParameteri(target, GLenum pname, GLint param) | |
```js | |
texParameterf(target, pname, param) |
(It will be a jssyntax
in the future, as soon as the PR allowing it in Prism lands.
for (object in scene) { | ||
let vertexList = ... | ||
let vertexList = "..."; |
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 it is a list. I think it should be:
let vertexList = "..."; | |
let vertexList = [/* ... */]; |
@@ -78,14 +78,14 @@ This renders a scene. But it's inefficient, because it allocates as local variab | |||
A simple change can optimize this significantly: | |||
|
|||
```js | |||
const vertexList = ... | |||
const vertexList = "..."; |
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.
const vertexList = "..."; | |
const vertexList = [/*...*/]; |
?
I'll use this comment to document some of the errors/intricacies I've come across but skipped for now: (POTENTIALLY OUTDATED, SEE #18186 (comment)):
|
I wonder if this won't be a deal-breaker if we want to automate ESLint? What do you think @Josh-Cena. How can we work around this problem? |
@teoli2003 It would be an issue, but we can (a) disable the linter on that block (b) split it (c) tell the linter to re-parse at RE: |
If Prism has it, we should add it and update Yari to use it (There is a list of languages it "loads"). |
For sure. I have yet to see a non-obscure language that Prism doesn't have a component for... |
// same as | ||
new URLPattern( | ||
'/books/:id', | ||
'https://example.com', | ||
); | ||
// or | ||
new URLPattern({ | ||
protocol: 'https', | ||
hostname: 'example.com', | ||
pathname: '/books/:id', | ||
}); | ||
// or | ||
new URLPattern({ | ||
pathname: '/books/:id', | ||
baseURL: 'https://example.com', | ||
}); | ||
``` |
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 would put const pattern =
… in front of each case.
Or const pattern1 =
and varying the 1.
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 first one was already using let pattern = ...
so I've adoped let
and went with your suggestion of naming the variables pattern[1..4]
So one comment left to address and this is good to go. |
This comment was marked as spam.
This comment was marked as spam.
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.
👍
Thanks a lot, we learned a lot thanks to this PR.
Summary
Followup PR to #18186
This PR…
-->