-
-
Notifications
You must be signed in to change notification settings - Fork 414
Replace hardcoded Assert URL with data-cite mechanism #5090
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for respec-pr ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
- Moved algorithms.js to run before inlines.js in all profiles - Updated algorithms.js to use data-cite="INFRA#assert" for linking - Updated tests to include localBiblio for INFRA spec Co-authored-by: marcoscaceres <[email protected]>
marcoscaceres
left a comment
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.
@copilot, exclude the builds/ in the commit. No need for those.
src/core/algorithms.js
Outdated
| html`<a href="https://infra.spec.whatwg.org/#assert">Assert</a>`, | ||
| ": " | ||
| ); | ||
| li.prepend(html`<a data-cite="INFRA#assert">Assert</a>`, ": "); |
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.
@copilot not quite. Just do and let ReSpec do the linking:
| li.prepend(html`<a data-cite="INFRA#assert">Assert</a>`, ": "); | |
| li.prepend(html`[=Assert=]:`); |
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.
Done in commit 0be2a94. Changed to use [=Assert=]: and added logic to add data-cite="INFRA" to the <li> element if infra is not already cited in the ancestor tree.
tests/spec/core/algorithms-spec.js
Outdated
| describe("Core — Algorithm Lists", () => { | ||
| afterAll(flushIframes); | ||
|
|
||
| const localBiblio = { |
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.
@copilot, no need to add this... the reference to infra needs to be added automatically.
What should happen is that you need to check if anything in the tree is already citing "infra", and if not, add data-cite="infra" to the closest parent node (i.e., the closest <li>)
| const localBiblio = { | |
| const localBiblio = { |
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.
Reverted the localBiblio changes and implemented the logic to check if data-cite="infra" already exists in the tree. If not, it gets added to the <li> element (commit 0be2a94).
marcoscaceres
left a comment
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.
@copilot some suggestions for you.
- Replace explicit anchor with [=Assert=] syntax for ReSpec linking - Add data-cite="INFRA" to li element if not already citing infra - Revert test file and builds folder to original state Co-authored-by: marcoscaceres <[email protected]>
Excluded builds/ from the commit by reverting them to the original state (commit 0be2a94). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
[=Assert=]:syntaxdata-cite="INFRA"to li element if infra not already cited in treeOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.