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

Flatten HTMLHyperlinkElementUtils mixin to HTMLAnchorElement and HTMLAreaElement #2046

Merged
merged 20 commits into from
Feb 11, 2021

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Feb 3, 2021

@Elchi3 Elchi3 requested review from a team as code owners February 3, 2021 15:18
@Elchi3 Elchi3 requested review from chrisdavidmills and removed request for a team February 3, 2021 15:18
@Elchi3
Copy link
Member Author

Elchi3 commented Feb 3, 2021

Reviewers: you won't see compat tables for the moment. mdn/browser-compat-data#8933 is the analog proof of concept in BCD land.

files/en-us/_redirects.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

I noticed one of the redirects looks weird. So I filed mdn/yari#2790
But I noticed a bunch of others that look equally weird. E.g.

/en-US/docs/Mozilla_event_reference/DOMContentLoading	(event)

/en-US/docs/URI/www	and

/en-US/docs/Web/Reference/Functions_and	classes_available_to_workers

I think it's a bug that the redirect validation stuff in CI didn't complain about these. So @Elchi3 would you mind manually look into those for now, whilst @fiji-flo and I worry about the validation script needing some improvements in mdn/yari#2790

@Ryuno-Ki
Copy link
Collaborator

Ryuno-Ki commented Feb 3, 2021

Am I assuming right, that a „Proof of concept” is not meant to be merged?
If so, could you mark this PR as draft to prevent an accidential merge?

@Elchi3 Elchi3 marked this pull request as draft February 3, 2021 17:40
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.

So this is all looking pretty damn good, Mr Scholz!

A few thoughts here:

  1. So for the redirects you are putting in place from the old mixin pages, what is the thought process here? Basically just redirect it to the likely most popular implementation of the mixin feature? It looks like you're redirected all the HTMLHyperlinkElementUtils pages to their HTMLAnchorElement, which makes sense as the <a> element is much more commonly used than <area>, but I just wanted to be sure of the logic here.

  2. Well, one page there doesn't seem to be a redirect present for is the actual /en-US/docs/Web/API/HTMLHyperlinkElementUtils interface page.

  3. on /en-US/docs/Web/API/HTMLAnchorElement and /en-US/docs/Web/API/HTMLAreaElement, password should be below origin for alphabetic order.

  4. Do you think the specification table on the interface pages needs to include a row for the mixin, i.e. pointing to https://html.spec.whatwg.org/multipage/links.html#htmlhyperlinkelementutils? I'm not sure about it. I mean, when you got to the spec definition for the interface itself, the WebIDL includes a line about the mixin anyway, e.g. HTMLAnchorElement includes HTMLHyperlinkElementUtils;

  5. Related to the above point, on member pages you have no mention anywhere that they are defined on a mixin rather than on the actual interface they appear on. I don't think it matters for the most part, but I reckon that we should at least have some kind of note in the specification table or somewhere that says (for example for http://localhost:5000/en-US/docs/Web/API/HTMLAnchorElement/origin) "In the specification, the origin property is defined on the HTMLHyperlinkElementUtils mixin". Most people won't care, but it'd be nice to clarify it for people that do.

  6. The standard syntax subsections (i.e. "Return value" for properties, "Parameters" and "Returns" for methods) are not included. Is this just for brevity while you do your experiment?

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 10, 2021

  1. So for the redirects you are putting in place from the old mixin pages, what is the thought process here? Basically just redirect it to the likely most popular implementation of the mixin feature? It looks like you're redirected all the HTMLHyperlinkElementUtils pages to their HTMLAnchorElement, which makes sense as the <a> element is much more commonly used than <area>, but I just wanted to be sure of the logic here.

Yeah, exactly this. I'm not sure it will be always like this but I thought it makes sense here.

  1. Well, one page there doesn't seem to be a redirect present for is the actual /en-US/docs/Web/API/HTMLHyperlinkElementUtils interface page.

I tried to do yarn content add-redirect /en-US/docs/Web/API/HTMLHyperlinkElementUtil /en-US/docs/Web/API/HTMLAnchorElement but as far as I can tell it removes the redirects for the subpages. I'm getting entries like these as a diff:

-/en-US/docs/Web/API/HTMLHyperlinkElementUtils/hash     /en-US/docs/Web/API/HTMLAnchorElement/hash

So, I thought having the individual pages redirect is more important than the main page redirect.

  1. on /en-US/docs/Web/API/HTMLAnchorElement and /en-US/docs/Web/API/HTMLAreaElement, password should be below origin for alphabetic order.

Done in 5041ded

  1. Do you think the specification table on the interface pages needs to include a row for the mixin, i.e. pointing to https://html.spec.whatwg.org/multipage/links.html#htmlhyperlinkelementutils? I'm not sure about it. I mean, when you got to the spec definition for the interface itself, the WebIDL includes a line about the mixin anyway, e.g. HTMLAnchorElement includes HTMLHyperlinkElementUtils;

As discussed in the other issue, I don't think this is needed. (also having in mind we want to auto-generate the spec table in the future).

  1. Related to the above point, on member pages you have no mention anywhere that they are defined on a mixin rather than on the actual interface they appear on. I don't think it matters for the most part, but I reckon that we should at least have some kind of note in the specification table or somewhere that says (for example for http://localhost:5000/en-US/docs/Web/API/HTMLAnchorElement/origin) "In the specification, the origin property is defined on the HTMLHyperlinkElementUtils mixin". Most people won't care, but it'd be nice to clarify it for people that do.

My feeling from the discussion is that a mixin is a concept that doesn't need to appear in docs necessarily. I don't think we need to worry about it at this stage. If there is demand that we should note that a feature comes to an interface via a mixin we can think about this. Right now, the idea is that it doesn't really matter and confuses everyone a lot when there is talk about a thing that only exists in the abstract / the spec.

  1. The standard syntax subsections (i.e. "Return value" for properties, "Parameters" and "Returns" for methods) are not included. Is this just for brevity while you do your experiment?

I copied from existing docs. Probably should have copied from a more current template. Sorry about that. Not sure it is mission-critical for this PR, though.

@Elchi3 Elchi3 changed the title Proof of concept: No longer document HTMLHyperlinkElementUtils mixin Flatten HTMLHyperlinkElementUtils mixin to HTMLAnchorElement and HTMLAreaElement Feb 10, 2021
@Elchi3 Elchi3 marked this pull request as ready for review February 10, 2021 14:25
@Elchi3
Copy link
Member Author

Elchi3 commented Feb 10, 2021

Merge conflicts resolved, redirect issue resolved, comments addressed. Marking as ready for review.

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.

I'm really happy wit h this. Let's get it merged!

@chrisdavidmills chrisdavidmills merged commit 31cd850 into mdn:main Feb 11, 2021
@Elchi3 Elchi3 deleted the poc-mixins branch February 11, 2021 17:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 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.

5 participants