-
Notifications
You must be signed in to change notification settings - Fork 75
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 Property Summary Data from MDN #49
Conversation
src/urls.ts
Outdated
} | ||
return ''; | ||
} catch (ex) { | ||
return '(Description not available)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this or should the string be empty and not generated into the JSDOC at all? Because properties that's missing compatibility data or the mdn_url
will be ignored too.
A warning in the terminal would be suitable though (as for when the summaryElement
is missing) to detect changes on their website since this solution is fairly fragile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we leave it empty, it will try to reach out during each build. I guess that would make it easier to automatically fix these as they get fixed on mdn.
If you are cool with that, an empty string would work fine here. I'll a console warning as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine due to the fact that the link is wrong and should be fixed at MDN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue mdn/browser-compat-data#1998
src/urls.ts
Outdated
console.log('fetching summary for ' + url); | ||
urlData[url] = summaryData = scrapeSummary(url) || ''; | ||
// todo: consider moving this to another part of the process | ||
saveToFile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine! 👍
|
I believe I have made all the changes we discussed. I ended up calling the summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot ❤️
PR for #48