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

Add MDN annotations to spec output #184

Merged
merged 4 commits into from
Oct 23, 2018
Merged

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Aug 24, 2018

This change adds annotations in the margin of the spec output next to any items for which there’s a corresponding MDN article somewhere under https://developer.mozilla.org/en-US/docs/Web that has a Specifications section with a link to that item's URL.

The mechanism for inserting the annotations relies on data in a JSON file containing a mapping of spec ID-attribute values to MDN article pathnames. The change adds a copy of that file to the repo, along with a makefile for updating/regenerating the JSON file.

Depends on whatwg/wattsi#89
Fixes #180

@sideshowbarker
Copy link
Contributor Author

https://arcane-cove-57093.herokuapp.com/multipage/ has sample output from this.

It currently adds 761 annotations

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/mdn-annotations branch from 8b5af7c to 0158882 Compare August 25, 2018 03:26
@domenic
Copy link
Member

domenic commented Aug 30, 2018

Interesting technique! The use of a Makefile is a fun choice... html-build can never have enough scripting languages, after all :). Can you add a README like we have for entities/ and quotes/?

Out of curiosity, how long does it take to run typically?

I wonder if we could create a Travis cron job to periodically run the makefile and send a pull request? (Also, we should probably be periodically running and updating the entities and quotes scripts, which long ago we stopped running every build...)

I'm happy to do that later, although I do think out of date MDN annotations would be sadder than out of date quote/entity stuff, so it'd be good to work on. @foolip has some experience with such things, I believe?

@foolip
Copy link
Member

foolip commented Aug 31, 2018

If updating the JSON file is expensive and we don't want to do it every time we build HTML, I think what I'd do is to create a daily Travis job to update it and automatically commit it.

@foolip
Copy link
Member

foolip commented Aug 31, 2018

This is really quite cool! Some things in https://arcane-cove-57093.herokuapp.com/multipage/form-elements.html where the rendering isn't quite perfect though.

Here the MDN things in the margin start to intrude too much on the text itself, even though it manages to avoid overlapping:
image

And here, the MDN thing overlaps the other info box we have in the margin:
image

@foolip
Copy link
Member

foolip commented Aug 31, 2018

Long term, it would be really cool if we could use https://github.com/mdn/browser-compat-data for the browser support instead, and essentially merge the two margin annotations into one.

@sideshowbarker
Copy link
Contributor Author

Interesting technique! The use of a Makefile is a fun choice... html-build can never have enough scripting languages, after all :).

Yeah and now it seems I’ll be adding some HTML parsing to this (in order to extract the article titles and class=seoSummary content.

Can you add a README like we have for entities/ and quotes/?

Yes, will do

Out of curiosity, how long does it take to run typically?

On my machine it takes about 21 minutes.

I wonder if we could create a Travis cron job to periodically run the makefile and send a pull request?

Yeah, certainly doable

(Also, we should probably be periodically running and updating the entities and quotes scripts, which long ago we stopped running every build...)

Yup

@sideshowbarker
Copy link
Contributor Author

If updating the JSON file is expensive and we don't want to do it every time we build HTML,

Yeah it’s very expensive (~21 minutes to build on my machine), so we definitely don’t want to do it every time we build HTML.

I think what I'd do is to create a daily Travis job to update it and automatically commit it.

Yeah. So having it be makefile-driven means we have an easy way to tell if there’s been any change. But then I guess git commit can tell too, even if the build doesn’t use a makefile

@sideshowbarker
Copy link
Contributor Author

This is really quite cool! Some things in https://arcane-cove-57093.herokuapp.com/multipage/form-elements.html where the rendering isn't quite perfect though.

So that’s why I made the annotations collapsible. If you click the MDN prefix, it hides the article-link hypertext. So you end up with just a MDN in the far-right margin. I guess I can tweak the styling of the MDN to add the ellipses thing that the caniuse annotations use — in order to make it more discoverable that the MDN parts are clickable, and the annotations are collapsible.

@sideshowbarker
Copy link
Contributor Author

I guess I can tweak the styling of the MDN to add the ellipses thing that the caniuse annotations use — in order to make it more discoverable that the MDN parts are clickable, and the annotations are collapsible.

I’ve gone ahead and done that.

If you scroll through https://arcane-cove-57093.herokuapp.com/multipage/ you can see the results.

@sideshowbarker
Copy link
Contributor Author

https://arcane-cove-57093.herokuapp.com/multipage/ has the output at this point (with the change made to use the article title and show the article summary on hover)

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/mdn-annotations branch 3 times, most recently from 3744ef9 to 95338a7 Compare September 13, 2018 13:47
@sideshowbarker
Copy link
Contributor Author

Long term, it would be really cool if we could use https://github.com/mdn/browser-compat-data for the browser support instead, and essentially merge the two margin annotations into one.

Agreed. And I think BCD is now evolving in a way that will eventually allow it to be used in the build here as the source for the MDN-article links/annotations (since as proposed in mdn/browser-compat-data#1531 (comment) it seems a spec_urls key will eventually get added to the BCD JSON).

So we’d be able to generate the MDN-article links/annotations without needing to scrape MDN to get the data, the way this patch is doing. And so without the ~20 minutes needed to do that scraping

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/mdn-annotations branch 2 times, most recently from 444232b to 5125d6b Compare September 23, 2018 13:32
This change adds annotations in the margin of the spec output next to
any items for which there’s a corresponding MDN article somewhere under
https://developer.mozilla.org/en-US/docs/Web that has a Specifications
section with a link to that item's URL.

The mechanism for inserting the annotations relies on data in a JSON
file containing a mapping of spec ID-attribute values to MDN article
pathnames. The change adds a copy of that file to the repo, along with a
makefile for updating/regenerating the JSON file.

Depends on whatwg/wattsi#89
Fixes #180
@sideshowbarker sideshowbarker force-pushed the sideshowbarker/mdn-annotations branch from 5125d6b to 677ed9e Compare October 4, 2018 05:37
@sideshowbarker
Copy link
Contributor Author

Here’s a ~15-second screen recording that shows the behavior of the annotations —

mdnannos720

@domenic
Copy link
Member

domenic commented Oct 20, 2018

So I'm still seeing problems related to these things not being direct children of body. For example on https://arcane-cove-57093.herokuapp.com/multipage/workers.html#handler-workerglobalscope-onerror trying to select "onerror" ends up selecting a bunch of MDN stuff as well. Let's please move all annotations to be <body> children.

After that, the only thing left is to move the CSS to a PR on whatwg/whatwg.org. At some point I'd like to play with the styles a bit to see if we can avoid overlap even better than what you currently have (e.g. I'm thinking a float: right + max-width + increased right margin?). I'm also worried about the overlap especially on mobile, but not sure how to solve that at the moment. But I think the current CSS is good enough for now.

@domenic
Copy link
Member

domenic commented Oct 20, 2018

Also: big fan of the new remote data files approach. And, can we omit these from review drafts? (Maybe make the Wattsi parameter optional?)

@sideshowbarker
Copy link
Contributor Author

So I'm still seeing problems related to these things not being direct children of body. For example on https://arcane-cove-57093.herokuapp.com/multipage/workers.html#handler-workerglobalscope-onerror trying to select "onerror" ends up selecting a bunch of MDN stuff as well.

OK, fixed and deployed to https://arcane-cove-57093.herokuapp.com/multipage/

And, can we omit these from review drafts?

Yes, fixed

(Maybe make the Wattsi parameter optional?)

That would take more work, because we’d really want to switch to using an actual options parser rather than continuing to use the ad-hoc way we’re parsing the options now.

So we really should change the options parsing but I’m not sure we should stop and try to do that now and block this PR on it. I think we could (should) do it after this PR is merged.

domenic added a commit to whatwg/whatwg.org that referenced this pull request Oct 21, 2018
domenic added a commit to whatwg/whatwg.org that referenced this pull request Oct 21, 2018
domenic added a commit to whatwg/build.whatwg.org that referenced this pull request Oct 23, 2018
Copy link
Contributor Author

@sideshowbarker sideshowbarker left a comment

Choose a reason for hiding this comment

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

LGTM

@domenic
Copy link
Member

domenic commented Oct 23, 2018

It looks like the build is broken now because sometimes it puts <aside> inside <h5>s :(. See https://travis-ci.org/whatwg/html-build/builds/445031916#L811 . Hopefully we can fix soon? Or I can roll back.

@sideshowbarker
Copy link
Contributor Author

It looks like the build is broken now because sometimes it puts <aside> inside <h5>s :(. See https://travis-ci.org/whatwg/html-build/builds/445031916#L811 .

Damn

Hopefully we can fix soon? Or I can roll back.

I’ll fix it right now

@domenic
Copy link
Member

domenic commented Oct 23, 2018

Oh no, I didn't see your comment here before I did whatwg/wattsi#91, I'm so sorry.

@domenic
Copy link
Member

domenic commented Oct 23, 2018

The build at https://travis-ci.org/whatwg/html-build/builds/445034036?utm_source=github_status&utm_medium=notification succeeds with Wattsi after whatwg/wattsi#91. So I think we can merge this, and make further changes to Wattsi that improve over my quick-fix.

@domenic domenic merged commit c29870d into master Oct 23, 2018
@domenic domenic deleted the sideshowbarker/mdn-annotations branch October 23, 2018 12:47
@sideshowbarker
Copy link
Contributor Author

Oh no, I didn't see your comment here before I did whatwg/wattsi#91, I'm so sorry.

No worries — better to have the build un-broken, and so far from looking at your code it all looks right.

So I think we can merge this, and make further changes to Wattsi that improve over my quick-fix.

Yup, I’ll take some time to test and see if it regressed anything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants