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

Remove Page macro in example sections - AudioListener #4401

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Apr 23, 2021

AudioListener properties and methods had the page macro in the Examples section that imported the example from BaseAudioContext/createPanner() (this was also included in AudioListener itself).

IMO an example should generally not be reproduced, but instead referenced. So I have replaced the inclusion with text like this:
image

@wbamberg @chrisdavidmills If you're OK with this approach I will roll it out to the similar cases.

This is part of addressing #3196

@hamishwillee hamishwillee requested a review from a team as a code owner April 23, 2021 04:48
@hamishwillee hamishwillee requested review from jpmedley, chrisdavidmills and wbamberg and removed request for a team and jpmedley April 23, 2021 04:49
@github-actions
Copy link
Contributor

github-actions bot commented Apr 23, 2021

Preview URLs

Flaws

Note! 15 documents with no flaws that don't need to be listed. 🎉

none! 🎉

External URLs

URL: /en-US/docs/Web/API/AudioListener
Title: AudioListener
on GitHub


URL: /en-US/docs/Web/API/AudioListener/dopplerFactor
Title: AudioListener.dopplerFactor
on GitHub


URL: /en-US/docs/Web/API/AudioListener/forwardX
Title: AudioListener.forwardX
on GitHub


URL: /en-US/docs/Web/API/AudioListener/positionX
Title: AudioListener.positionX
on GitHub


URL: /en-US/docs/Web/API/AudioListener/speedOfSound
Title: AudioListener.speedOfSound
on GitHub


URL: /en-US/docs/Web/API/AudioListener/upZ
Title: AudioListener.upZ
on GitHub


URL: /en-US/docs/Web/API/AudioListener/positionZ
Title: AudioListener.positionZ
on GitHub


URL: /en-US/docs/Web/API/AudioListener/upX
Title: AudioListener.upX
on GitHub


URL: /en-US/docs/Web/API/AudioListener/setPosition
Title: AudioListener.setPosition()
on GitHub


URL: /en-US/docs/Web/API/AudioListener/upY
Title: AudioListener.upY
on GitHub


URL: /en-US/docs/Web/API/AudioListener/forwardZ
Title: AudioListener.forwardZ
on GitHub


URL: /en-US/docs/Web/API/AudioListener/positionY
Title: AudioListener.positionY
on GitHub


URL: /en-US/docs/Web/API/AudioListener/forwardY
Title: AudioListener.forwardY
on GitHub


URL: /en-US/docs/Web/API/AudioListener/setOrientation
Title: AudioListener.setOrientation()
on GitHub

No external URLs

URL: /en-US/docs/Web/API/BaseAudioContext/createPanner
Title: BaseAudioContext.createPanner()
on GitHub

(this comment was updated 2021-04-26 03:19:39.896079)


<h3 id="Value">Value</h3>

<p>An {{domxref("AudioParam")}}. Its default value is 0, and it can range between positive and negative infinity.</p>

<h2 id="Example">Example</h2>

<p>{{page("/en-US/docs/Web/API/AudioListener","Example")}}</p>
<pre class="brush: js">let audioCtx = new AudioContext();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wbamberg @chrisdavidmills This one I have treated as a special case "for discussion".

This pulls down the (old-style) syntax section which shows how to use the property in a kind of summary form. Is it useful to provide a summary example like this and then link to the full/practical example. Or is this not worth thinking about?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well first, I do think your PR is an improvement on what we have: I agree that linking is better than embedding here. So if that's all we do now it's still an improvement, and we should still do it.

I think this syntax example is not very helpful, because it doesn't tell me anything about why I might want to set this or what the effect will be. It even sets it to 0, which is the default anyway, so I just think, why?

If we're going to have an example for this particular property, minimally I think it should set it to something non-default and explain what the effect of that would be.

Still, maybe this is better than nothing?

But I think the examples for this interface are not in general ideal: linking all the individual pages to one big example, which itself is just a piece of an even bigger example, means I have to digest the whole big example before I can understand the pieces - I have to learn everything before I can learn anything. It would be more helpful to have individual examples for the different pieces that can be understood in isolation, along with bigger examples that put them together.

Specifically for the AudioListener, it looks like even the bigger example never modifies the listener values.

But you know, I appreciate this is much easier said than done, and this is a complex API with lots of interdependent parts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wbamberg Thanks very much. I agree with your points:

  • The syntax copied down to the example box is not very useful, but is perhaps better than nothing in this case.
  • The big example may require you to understand more context than you need to just understand just one of these attributes. Though I don't think it is so big that it is undigestible in this case.
  • The big example isn't all that relevant for many of the attributes it is used in. However it does give you some idea.

It would be possible to go through this API and create individual examples that are more appropriate. The big example is still very relevant and would be linked from almost all cases either in the example section or a See also section.

Note though that if the examples were bad before then they are still bad, and if good before they are still good. The question is whether this is something to fix properly in detail as we go or whether you want to get rid of the page macro - which is all I was trying to achieve (+ being "no worse than before)?

Obviously I made the call to just remove the macro. Happy to go the other way if you feel that it is worth it? Will slow things down very considerably.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I agree with all this, I'm just grumbling really. Let's keep the scope to getting rid of {{page}}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:-). OK, I removed the "syntax example", and created an issue to track fixing this "eventually": #4468

This PR is OK for you then to review/merge as you will. Discussion with Florian can continue even after than happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

OK, let's do this!

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