-
Notifications
You must be signed in to change notification settings - Fork 439
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
API Descriptions Update #1940
base: main
Are you sure you want to change the base?
API Descriptions Update #1940
Conversation
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
We still depend on that, so we can't just remove it until we get a new mdn/content dependency? |
I know |
Hello @saschanaz, |
Maybe |
Hello:) |
I mean |
|
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.
Almost there!
Done @saschanaz |
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.
The only blocking change is the workflow thing, otherwise this looks mostly fine. 🎉
Done @saschanaz |
Done @saschanaz |
Hello @saschanaz, My PR to the MDN repo has been merged, and I’ve synced the repo to resolve the abbreviation issue. |
LGTM, thanks! (This may also require Microsoft review, but let me try given there's no maintainers tag...) |
Sorry @saschanaz, you don't have access to these files: |
^ @sandersn, can you take a look? |
Hello @sandersn |
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.
One tiny cleanup request.
I'm not a fan of submodules but if @saschanaz is OK with the overhead of working with them it's fine with me.
submodules: true # Ensure submodules are checked out | ||
|
||
- name: Update submodules | ||
working-directory: inputfiles/mdn # Replace with actual submodule path |
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.
these comments look left over from an example, since inputfiles/mdn
is the actual submodule path. Can you remove them?
I considered adding it as a npm package but that causes a terribly deep dependency tree, so I think submodule is the best choice for now. |
Thank you @sandersn for your review. I have updated the workflow |
Hello @sandersn, I understand you worry that it won't be updated regularly because the update dependencies workflow will auto update it. So I don't think you should worry about it 🙂 |
fixes #1937