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

Adding at() docs #1216

Merged
merged 21 commits into from
Jan 25, 2021
Merged

Adding at() docs #1216

merged 21 commits into from
Jan 25, 2021

Conversation

Rumyra
Copy link
Collaborator

@Rumyra Rumyra commented Jan 12, 2021

Adding array.at(), string.at() and typedarray.at() pages.

Looking at a review as these are actually my first js ref docs... some notes:

tl:dr; please review but don't merge until I've resolved above :) @chrisdavidmills

@Rumyra Rumyra requested a review from a team as a code owner January 12, 2021 12:30
@Rumyra Rumyra requested review from sideshowbarker and removed request for a team January 12, 2021 12:30
@Rumyra Rumyra marked this pull request as draft January 12, 2021 12:31
Copy link
Member

@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.

Nits

@chrisdavidmills
Copy link
Contributor

Hi @Rumyra !

I'm just preparing for the Fx 85 launch and giving this a review as part of it.

Looks mostly good. Any nitpicks I find I'll make directly to the text and not both you with them.

A few points to help you with the remainder:

Can you make this a priority? The launch of Fx 85 is in 8 days. Thanks!

@Rumyra
Copy link
Collaborator Author

Rumyra commented Jan 20, 2021

I've updated the spec links and bcd... one thing is the at method is not in the draft spec yet 😬 I've added what should be the link...

@Rumyra Rumyra marked this pull request as ready for review January 20, 2021 11:36
@chrisdavidmills
Copy link
Contributor

one thing is the at method is not in the draft spec yet 😬 I've added what should be the link...

Hrm, this makes me wonder whether we shouldn't just link to the draft proposal instead for now.

@Elchi3 , what would you do here? at() is currently only specified at https://tc39.es/proposal-relative-indexing-method/, and not in the ESDraft spec yet, so we are not sure what to do about the spec link.

@Rumyra
Copy link
Collaborator Author

Rumyra commented Jan 20, 2021

Let me know - I'll update here if it needs changing 👍

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

This is great! Happy to see you contributing to JS ref docs. I've reviewed this some more and left comments on the Array.prototype.at page for you, but they also apply to the other two methods mostly.

@Rumyra
Copy link
Collaborator Author

Rumyra commented Jan 20, 2021

Converting to draft until I've done feedbacks

@Rumyra Rumyra marked this pull request as draft January 20, 2021 16:08
@Rumyra Rumyra marked this pull request as ready for review January 22, 2021 12:19
@Rumyra Rumyra requested a review from Elchi3 January 22, 2021 15:55
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Great stuff! Some nits and I think we're good to go.

@Rumyra Rumyra requested a review from Elchi3 January 25, 2021 13:08
@Elchi3 Elchi3 merged commit 2b09e00 into mdn:main Jan 25, 2021
@Rumyra Rumyra deleted the atdocs branch March 2, 2021 14:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 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.

4 participants