-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix nav links of synonymous sub-headings under Associations #12201
Conversation
Not sure how to add test for this one. |
Codecov Report
@@ Coverage Diff @@
## master #12201 +/- ##
=======================================
Coverage 96.33% 96.33%
=======================================
Files 95 95
Lines 9120 9120
=======================================
Hits 8786 8786
Misses 334 334 Continue to review full report at Codecov.
|
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.
Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!
Thanks for catching the fact that cheerio was missing from devDependencies
! 🚀 I either deleted it by accident at some point, or never added it 😅
As for your PR, although it does work, I was hoping for something less specific, that would automatically adapt to further documentation changes.
Would you be willing to improve your PR? I suggest something on the lines of:
- Detect every heading with an ID:
$('h1,h2,h3,h4,h5').filter('[id]')
- Find duplicate IDs among them
- Replace their IDs with the following rule:
#original-header --> #original-header #original-header --> #original-header-2 #original-header --> #original-header-3
- Also fix nav links (nice catch, I didn't think of that!) - you can assume that the links appear in nav in the same order they do in the page itself. To find the links you might use something like
$('li[data-ice="manualNav"] a').filter(function() { return $(this).prop('href').endsWith(`${fileName}#${headerBeingFixed}`); });
Not sure how to add test for this one.
Don't worry about a test, I think it's not necessary in this case, provided we manually check that it works.
Got confused by the mechanics of |
Also apparently |
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.
LGTM, waiting for @papb
Great work @louy2, thank you very much!! |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Added new transform
fix-ids.js
to fix sub-heading ids and links under Association.Closes #12197