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

New introduction for Mapped Types article #2346

Open
wants to merge 9 commits into
base: v2
Choose a base branch
from

Conversation

daiplusplus
Copy link
Contributor

The current introduction in the Mapped Types article is unclear (and what's the relevance of an Only Fools and Horses reference?): the example given didn't really demonstrate what "mapping" one type to another meant by explicit example.

The current introduction in the Mapped Types article is unclear (and what's the relevance of an _Only Fools and Horses_ reference?): the example given didn't really demonstrate what "mapping" one type to another meant by explicit example.
@daiplusplus
Copy link
Contributor Author

@orta sorry for pinging you, but as the main active contributor to TS's docs, do you know what's holding-up review and approval of my PR?

@orta
Copy link
Contributor

orta commented Jun 7, 2022

Yeah, I looked at it but removing the only fools and horses reference kinda dinged your PR from my perspective, keeping any sense of whimsy and light heartedness in the docs is a battle that sometimes I'm just not in the mood to have. We get good PRs like this that are a net positive for the individual topic but slowly make the docs really dry when going through the whole handbook.

Which makes giving feedback hard because I think it documents the syntax well, and this page is effectively the reference for the syntax even though it lives in the handbook. So I think I land on the side of losing that is a shame, it's probably worth it in this case.

WRT it getting merged though, that's on someone with write access now I'm afraid and I think the team are still figuring out what they want there.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

One thing I'm unsure of in this change is that the intro now effectively explains everything in one go, specifically the next "mapping modifiers" section. I'm not sure if that's good or bad, without getting others to look and see what might be good for first time readers.

@jakebailey
Copy link
Member

FYI, we can't really consider taking this without the CLA stuff above; are you still interested in finishing this PR?

@daiplusplus
Copy link
Contributor Author

daiplusplus commented Aug 2, 2023

@microsoft-github-policy-service agree

@jakebailey I renamed my GitHub account within the past year, looks like the CLA-Bot isn't aware of renamed GitHub accounts - is it stuck?

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Some wording tweaks.

@jakebailey jakebailey closed this Jun 5, 2024
@jakebailey jakebailey reopened this Jun 5, 2024
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
@daiplusplus
Copy link
Contributor Author

@jakebailey Sorry I forgot about this PR for almost a year. I've accepted your proposed changes as-is (for the sake of expediency) - there are some minor changes I'd have liked to have made or added now, but GitHub's online editor just wasn't working for me today - but anyway.

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

Successfully merging this pull request may close these issues.

4 participants