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

Editorial: Align with the wording used elsewhere. #3097

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zhangenming
Copy link
Contributor

reduce reading distractions

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM

spec.html Outdated
Comment on lines 34261 to 34266
<emu-note>This method is intentionally generic, it does not require that its *this* value be a String object. Therefore, it can be transferred to other kinds of objects for use as a method.</emu-note>
<emu-note>Similarly to `String.prototype.split`, `String.prototype.matchAll` is designed to typically act without mutating its inputs.</emu-note>
<emu-note>
<p>This method is intentionally generic; it does not require that its *this* value be a String object. Therefore, it can be transferred to other kinds of objects for use as a method.</p>
</emu-note>
<emu-note>
<p>Similarly to `String.prototype.split`, `String.prototype.matchAll` is designed to typically act without mutating its inputs.</p>
</emu-note>
Copy link
Member

Choose a reason for hiding this comment

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

let's revert all the lines that aren't changing any words, only changing indentation and whitespace

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'm sorry I didn't get it. What exactly do I need to do.
Other place has a p tag embedded in it, so I added it here.

Copy link
Member

Choose a reason for hiding this comment

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

There's no need for that, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(An <emu-note> with a single paragraph of content usually puts it in a <p> element: ~550 have it vs ~20 don't.)

Copy link
Member

Choose a reason for hiding this comment

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

ah, fair enough

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

This PR might also include the two "intentionally generic factory method" notes in its scope.

Re changing

<emu-note>
  <p>Foo</p>
</emu-note>

to just

<emu-note>Foo</emu-note>

It's 'valid', but I'm not sure it's an improvement. (@michaelficarra's thumbs-up on #3097 (comment) suggests a preference to use the former, but you might as well wait for an explicit indication.)

The other changes (punctuation, wording, blank-line removal) seem okay to me.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
Comment on lines 34238 to 34240
<emu-note>
<p>Similarly to `String.prototype.split`, `String.prototype.matchAll` is designed to typically act without mutating its inputs.</p>
</emu-note>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The change to this note seems out-of-place in this PR.

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Seems okay to me.

I'm wondering if you missed this comment:

This PR might also include the two "intentionally generic factory method" notes in its scope.

@ljharb
Copy link
Member

ljharb commented Dec 22, 2023

This needs to be rebased (not closed and reopened, please)

@zhangenming zhangenming reopened this Apr 8, 2024
@zhangenming zhangenming requested a review from jmdyck April 8, 2024 11:12
@zhangenming
Copy link
Contributor Author

done? But I don't know why he turned it off automatically and then I had to turn it back on

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

The Notes for:

  • String.prototype.matchAll
  • Array.from
  • Array.of

are quite similar to the ones addressed by this PR, and might as well be made consistent too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants