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

Update JS Array syntax sections to avoid BNF syntaxes #4136

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Apr 15, 2021

In this PR, I've tried to play through the new guidelines for MDN's syntax sections for the JS Array built-in. So, here I:

  • got rid of a lot of <var> in <pre>
  • used multiple lines to show optional parameters
  • removed any formal syntax notations (BNF)

See https://developer.mozilla.org/en-US/docs/MDN/Structures/Syntax_sections#constructors_and_methods for the new guidelines and openwebdocs/project#26 and the discussion at #2202.

There are a few methods here that use callbacks and there we lack clear guidelines on what to do but I tried to make these better by demonstrating how to use it with arrow functions, functions, and inline functions. I'm especially looking for review feedback on this aspect.

@Elchi3 Elchi3 requested a review from a team as a code owner April 15, 2021 15:27
@Elchi3 Elchi3 requested review from sideshowbarker and removed request for a team April 15, 2021 15:27
Copy link
Collaborator

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

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

🧡 Huge improvement IMHO. I love this. Approving since the suggested changes are all just typo fixes. With to nits fix, I would be very happy to see this get merged.

@sideshowbarker
Copy link
Collaborator

There are a few methods here that use callbacks and there we lack clear guidelines on what to do but I tried to make these better by demonstrating how to use it with arrow functions, functions, and inline functions. I'm especially looking for review feedback on this aspect.

That part all works very well. I don’t have any more specific feedback than that. I think it’s a significant improvement in clarity and usability.

@@ -14,10 +14,15 @@

<h2 id="Syntax">Syntax</h2>

<pre class="brush: js">[<var>element0</var>, <var>element1</var>, ..., <var>elementN</var>]
<pre class="brush: js">
// literal constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

FMI, generally I use sentence case for comments. Is there a standard on this?

<pre
class="brush: js">const <var>new_array</var> = <var>old_array</var>.concat([<var>value1</var>[, <var>value2</var>[, ...[, <var>valueN</var>]]]])</pre>
<pre class="brush: js">
concat()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it is valid, but I do wonder what it would "mean".

@@ -19,8 +19,22 @@

<h2 id="Syntax">Syntax</h2>

<pre
class="brush: js"><var>arr</var>.every(<var>callback</var>(<var>element</var>[, <var>index</var>[, <var>array</var>]])[, <var>thisArg</var>])</pre>
<pre class="brush: js">
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW this works really well.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

I'm approving too. I'd hit the merge button, but not sure if you're looking for broader feedback.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Apr 19, 2021

Actually, screw it. Merging. I'd rather ask forgiveness.

@Elchi3 Can you update the how to docs to capture the n element case?

@Elchi3
Copy link
Member Author

Elchi3 commented Apr 19, 2021

@Elchi3 Can you update the how to docs to capture the n element case?

Opened #4249

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants