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 URL API #1122

Merged
merged 13 commits into from Mar 8, 2018
Merged

Add URL API #1122

merged 13 commits into from Mar 8, 2018

Conversation

@Elchi3 Elchi3 added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 18, 2018
@Elchi3 Elchi3 added this to In Progress / Review Needed in Migration of compat data from MDN pages Feb 18, 2018
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.

Hey, thanks for your pull request! Welcome to mdn-browser-compat-data. Sorry for not coming back to you earlier, the repo is quite busy.

I left some comments and they also apply to else where in the file. Can you fix these?
Also the constructor is missing as a sub feature here, https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

Thanks for your work!

api/URL.json Outdated
{
"version_added": "2",
"alternative_name": "webkitURL",
"notes": "This feature is implemented under the non-standard name webkitURL."
Copy link
Member

Choose a reason for hiding this comment

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

This note is redundant. Please remove

api/URL.json Outdated
"notes": "This feature is implemented under the non-standard name webkitURL."
},
{
"version_added": "32"
Copy link
Member

Choose a reason for hiding this comment

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

Can you swap this around to be reverse chronological, please?
[
{ version_added: 32 },
{ version_added: 2, alternative_name: webkitURL}
]

api/URL.json Outdated
"notes": "From Gecko 2 (Firefox 4) to Gecko 18 included, Gecko supported this interface with the non-standard nsIDOMMozURLProperty internal type. As the only to access such an object was through window.URL, in practice, this didn't make any difference."
},
{
"version_added": "19"
Copy link
Member

Choose a reason for hiding this comment

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

Reverse chronological please. version 19 is the most relevant version that should appear in the main cell.

api/URL.json Outdated
"version_added": "15",
"alternative_name": "webkitURL",
"notes": [
"This feature is implemented under the non-standard name webkitURL."
Copy link
Member

Choose a reason for hiding this comment

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

note is redundant, please remove

api/URL.json Outdated
]
},
{
"version_added": "19"
Copy link
Member

Choose a reason for hiding this comment

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

This needs to come first

api/URL.json Outdated
"version_added": "6",
"alternative_name": "webkitURL",
"notes": [
"This feature is implemented under the non-standard name webkitURL."
Copy link
Member

Choose a reason for hiding this comment

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

remove

api/URL.json Outdated
]
},
{
"version_added": "7"
Copy link
Member

Choose a reason for hiding this comment

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

Put this version first

@enjikaka
Copy link
Contributor Author

enjikaka commented Mar 7, 2018

@Elchi3
Copy link
Member

Elchi3 commented Mar 8, 2018

Wow, nice work! I really like the idea of little helper scripts. Would you mind putting that into a new PR, though? Allows us to have a separate discussion about how we want to add this.

Looks good otherwise except that the constructor is still missing as a sub feature, https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

@enjikaka
Copy link
Contributor Author

enjikaka commented Mar 8, 2018

Wow, nice work! I really like the idea of little helper scripts. Would you mind putting that into a new PR, though? Allows us to have a separate discussion about how we want to add this.

I'll make a new PR with that! :)

as a sub feature

I don't fully understand. Isn't the first __compat the constructor? 🤔

@Elchi3
Copy link
Member

Elchi3 commented Mar 8, 2018

The first __compat is the URL interface itself (and some interfaces have no constructor).

@enjikaka
Copy link
Contributor Author

enjikaka commented Mar 8, 2018

@Elchi3 Ok, I'll look over the file again and try to understand what to change and get it sorted! Meanwhile; here's a PR for the helper script: #1314

@Elchi3
Copy link
Member

Elchi3 commented Mar 8, 2018

Thanks!
Have a look at other APIs, like https://github.com/mdn/browser-compat-data/blob/master/api/AnalyserNode.json which also has a constructor (api.AnalyserNode.AnalyserNode).

@enjikaka
Copy link
Contributor Author

enjikaka commented Mar 8, 2018

@Elchi3 Added constructor now!

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.

Awesome, thank you!

@Elchi3 Elchi3 merged commit babbb0d into mdn:master Mar 8, 2018
Migration of compat data from MDN pages automation moved this from In Progress / Review Needed to MDN page updates needed Mar 8, 2018
@enjikaka enjikaka deleted the URL branch March 8, 2018 14:12
@Elchi3 Elchi3 moved this from MDN page updates needed to Done in Migration of compat data from MDN pages Mar 9, 2018
dontcallmedom pushed a commit to dontcallmedom/browser-compat-data that referenced this pull request Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants