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

Fix content and display bug on videoWidth page #4059

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

amyrlam
Copy link
Contributor

@amyrlam amyrlam commented Apr 13, 2021

What was wrong/why is this fix needed? (quick summary only)

  • Page had broken anchor tag, switch to more informative external link
  • Page had duplicate macro that incorrectly showed up as macro text itself

MDN URL of main page changed

https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/videoWidth

Issue number (if there is an associated issue)

Resolves: #1657

Anything else that could help us review it

I'm not sure how to run things locally (is that expected? didn't see it in the README or elsewhere) so am trying this since the Preview URL on #4058 doesn't seem to reflect changes made in later commits(?). Not sure what I'm missing yet.

@amyrlam amyrlam requested a review from a team as a code owner April 13, 2021 08:37
@amyrlam amyrlam requested review from Elchi3 and removed request for a team April 13, 2021 08:37
@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@@ -21,8 +21,7 @@
<p><span class="seoSummary">The {{domxref("HTMLVideoElement")}} interface's read-only
<code><strong>videoWidth</strong></code> property indicates the <strong>intrinsic
width</strong> of the video, expressed in CSS pixels. In simple terms, this is the
width of the media in its natural size.</span> See {{anch("About intrinsic width and
height")}} for more details.</p>
width of the media in its natural size.</span> See {{Glossary("Intrinsic size")}} for more details.</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is broken on the Preview URL https://pr4059.content.dev.mdn.mozit.cloud/en-US/docs/Web/API/HTMLVideoElement/videoWidth, but exists https://developer.mozilla.org/en-US/docs/Glossary/Intrinsic_Size, what am I missing?

I changed the casing before from "Intrinsic size" to "Intrinsic Size" but didn't see an update on the Preview URL on the prior PR. Where to learn more about the macros?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about the Preview URLs. This preview just landed the other week and there were problems with caching. Likely why you didn't see updates.

Copy link
Member

Choose a reason for hiding this comment

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

So, this means to link to the section that the {{page}} macro below should render but doesn't. I see you fixed that {{page}} macro call, so I reckon we can have the anchor back. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Elchi3 Thank you for your help!

Sounds good, made that change. I'm stumped why the link has an extra _ though?

image

Copy link
Member

@Elchi3 Elchi3 Apr 19, 2021

Choose a reason for hiding this comment

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

How strange?! Is it because there is a line break in line 24 and 25 in the middle of the {{anch}} macro call? Can you try making that one line and see if that fixes it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Elchi3 Tried that and undid the commit. FWIW it's wrong on production https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/videoWidth#about_intrinsic_width_and__height, seems like a legit bug with anch.

I can change the heading to "About" (one word) or something, but wasn't sure how you want to proceed?

Also, I can file an issue / look into it if you can advise where it is? Took a quick look at mdn on GitHub but didn't know.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's so weird! We don't really like our macros and so we could also avoid the macro call altogether and use good old HTML (we plan to convert the HTML to markdown and so the less macros the better).

The anch code lives here: https://github.com/mdn/yari/blob/main/kumascript/macros/anch.ejs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Elchi3 aha, yes migrating to HTML sounds great. Thanks for sharing the macro anyways!

The fix looks good on the latest preview URL #4059 (comment), and I squashed my commits, take a look at your convenience.

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 13, 2021

One last note, thanks for bearing with me: I can fix the broken link to resize from #4059 (comment) also (can see on https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/videoHeight, resize is in red, does not exist) but not sure if there should be a link, or if the link should simply be removed?

@Ryuno-Ki
Copy link
Collaborator

Ping @Elchi3.
I'm not that comfortable with the Macros edited in this PR.

@Elchi3
Copy link
Member

Elchi3 commented Apr 16, 2021

One last note, thanks for bearing with me: I can fix the broken link to resize from #4059 (comment) also (can see on https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/videoHeight, resize is in red, does not exist) but not sure if there should be a link, or if the link should simply be removed?

hm interesting. Not sure if HTMLVideoElement has a dedicated resize event, so I'm also unclear what this is trying to refer to. Feel free to ignore that for this PR, though. Might be good to raise an issue about it, however.

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 19, 2021

@Elchi3 I made a separate issue for the broken resize link here: #4230.

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 3

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist
    • `/home/runner/work/content/content/node_modules/@mdn/yari/kumascript/macros/page.ejs:23
      21| }
      22| %>

23| <%- await wiki.page(url, $1, $2, $3, $4) %>
24|

unable to find an HTML element with an "id" of "About_intrinsic_width_and__height" within /en-us/docs/web/api/htmlvideoelement/videoheight`

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

Resolves: mdn#1657

Use HTML instead of anch macro
@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/resize does not exist
    • /en-US/docs/Web/API/HTMLVideoElement/resize does not exist

External URLs

URL: /en-US/docs/Web/API/HTMLVideoElement/videoWidth
Title: HTMLVideoElement.videoWidth
on GitHub

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.

Thanks for your patience with this one, @amyrlam! Congrats on landing your first PR to MDN! 🎉

@Elchi3 Elchi3 merged commit f432ae5 into mdn:main Apr 20, 2021
@amyrlam amyrlam deleted the bug/video-width-page-2 branch April 20, 2021 16:55
@amyrlam
Copy link
Contributor Author

amyrlam commented Apr 20, 2021

Thank you @Elchi3 for all of your help!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 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.

Issue with "HTMLVideoElement.videoWidth": …
3 participants