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

BatteryManager: Incorporate methods rather than include #3388

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

hamishwillee
Copy link
Collaborator

Fixes #3316

Copy-paste of methods from EventTarget, and removal of additional chrome/mozilla methods.

@wbamberg Set you as reviewer for this first one.

@hamishwillee hamishwillee requested a review from a team as a code owner March 23, 2021 02:38
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Sorry if I wasn't clear in #3316. I don't think the fix here is to copy/paste the EventTarget methods, I think it's just to point to EventTarget.

If you look at, say, https://developer.mozilla.org/en-US/docs/Web/API/AudioContext#methods, it just says:

Also inherits methods from its parent interface, BaseAudioContext.

...it doesn't copy/paste those methods into this page.

(There is quite a lot of variability in how we handle inheritance across the Web/API docs, but this - pointing to the parent but not copying the list of features - is the most common. And even within this general approach there's a lot of variation in the exact words (see e.g. https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement#methods). It would be great to have more consistency here, but that's probably out of scope for this bug :).)

@hamishwillee
Copy link
Collaborator Author

@wbamberg Arrgg, you were clear, but I clicked another example :-(.

I have fixed this up in-line with the same approach. I see no reason we can't propose some standard text for fixes to at least reduce the variation.

Minor point, but should we link direct to the methods section? It makes little difference here, but if you have a big base class then you'll be looking around for it. Note that to do this we can't use {{domxref("EventTarget","EventTarget","Methods")}} - the link works but it renders as EventTarget.Methods

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @hamishwillee !

I see no reason we can't propose some standard text for fixes to at least reduce the variation.

We can propose it, but without linters to check I doubt it would be very effective. But maybe I'm just bitter :).

I think linking to #methods is a good idea, but again it would be a thing we'd want to do across all the docs.

@wbamberg wbamberg merged commit 2ef00f4 into mdn:main Mar 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 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.

Content bug: BatteryManager includes "Additional methods in Mozilla chrome codebase" from EventTarget
2 participants