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 contribtion docs for new mixin guideline #2366

Merged
merged 4 commits into from
Feb 16, 2021

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Feb 15, 2021

See #1940 for background.

Co-authored-by: Michael[tm] Smith <[email protected]>
@sideshowbarker
Copy link
Collaborator

Not merging since as project-meta docs, it seems this should have review and OK from @chrisdavidmills

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 16, 2021

Thanks Mike and yup, Chris said yesterday he'll have a look.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Mostly looking good here, Mr @Elchi3 ! I just had one bit of feedback for you.

Changelog:

  • The content you've added here is not wrong, but it feels a bit thin on the ground. It feels to me like you should provide a bit more information — what it used to be like and what it is like now — and link to the document section that explains how to document a mixin.

So something like "Previously we commonly defined a landing page for the mixin class itself, and put the defined members on subpages underneath it, before linking to those from the landing pages of the interfaces that implement those mixins. This was confusing for readers because mixins are spec constructs — you never access the defined members using the mixin classes. To avoid this confusion we've instead put the pages for members defined on mixins directly under the implementing class pages. For more details, see XXXXXXX"

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Much better, thanks @Elchi3 !

So, small copy edit, and we're done. Merging now.

@chrisdavidmills chrisdavidmills merged commit 8f6de46 into mdn:main Feb 16, 2021
@Elchi3 Elchi3 deleted the contrib-docs-mixins branch February 16, 2021 14:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 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