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

Adds basic WebRTC types and cleans up build scripts #7

Merged
merged 6 commits into from
Dec 5, 2017
Merged

Adds basic WebRTC types and cleans up build scripts #7

merged 6 commits into from
Dec 5, 2017

Conversation

SupremeTechnopriest
Copy link
Contributor

Hello, thanks for documentation.js. It's great and I use it everywhere.

I wanted to have my WebRTC types link to their respective mozilla documentations. After poking around I ended up here. When I cloned the repo I saw some things in the build chain that didn't quite make sense so I decided to tidy up. Please review my commits and make sure we are on the same page. Tests all pass, npm run build works to add new browser features and WebRTC links are all correct.

Thanks!

This build directory made no sense to me. It was requiring a non
existent file and writing to an un-used/ un-exported file.  Also the
source file was not exported or used anywhere that I could see. To make
building better I made the following changes:

- rename the `build` directory to `scripts` as `build` is misleading
- remove the file `source.json`
- propagate the last pull request #4 to the build script
- add an npm command to build `npm run build`

Now running `npm run build` will use the file `globals-docs.json` as its
input and output file.  To add more browser features just set a key to
the feature name and the value to https://developer.mozilla.org and run
`npm run build` to resolve the mozilla documentation and generate a new
`globals-docs.json`
@SupremeTechnopriest
Copy link
Contributor Author

I went ahead and full refactored build to solve #5. All the SVG types resolve to documentation now, but there are still a few outliers like name and opera

@SupremeTechnopriest
Copy link
Contributor Author

SupremeTechnopriest commented Dec 5, 2017

It is worth noting that this last commit changes the default value from https://developer.mozilla.org to https://developer.mozilla.org/ (added slash). If you wanted to add another browser feature say RTCConfiguration, you would add a property to globals-docs.json like this:

{
    RTCConfiguration: "https://developer.mozilla.org/",
}

and then run npm run build to resolve to the actual links.

@@ -283,13 +290,13 @@
"SVGAnimationElement": "https://developer.mozilla.org/docs/Web/API/SVGAnimationElement",
"SVGCircleElement": "https://developer.mozilla.org/docs/Web/API/SVGCircleElement",
"SVGClipPathElement": "https://developer.mozilla.org/docs/Web/API/SVGClipPathElement",
"SVGColor": "https://developer.mozilla.org/",
"SVGColor": "https://hacks.mozilla.org/2017/10/containers-for-add-on-developers/",
Copy link
Member

Choose a reason for hiding this comment

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

This one isn't quite right. Maybe it's time that we should just manually manage this list instead of relying on Google entirely for scrying the right URLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't be the end of the world... This is a bit more complete then what we have now anyway. Your call.

Copy link
Member

Choose a reason for hiding this comment

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

👍 afaict no browser actually has a SVGColor global, so it should be fine to remove this line entirely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

patch incoming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.gitignore Outdated
@@ -0,0 +1,3 @@
*.log
Copy link
Member

Choose a reason for hiding this comment

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

Mind keeping this out of the repo? I prefer to keep application-related logs in a global .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure no problem. Give me a second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set 👍

@tmcw tmcw merged commit 84f7df0 into documentationjs:master Dec 5, 2017
@SupremeTechnopriest
Copy link
Contributor Author

Thanks! How long before the changes will reflect in documentation.js?

@tmcw
Copy link
Member

tmcw commented Dec 5, 2017

If you're installing with npm, should be right now because documentation.js uses ^2.3.0 to specify globals-docs. But I need to roll a release with a newer yarn.lock and updated tests to make sure everyone gets newer globals-docs versions.

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

Successfully merging this pull request may close these issues.

2 participants