fix: normalize p5 reference links in parameter sections - #1284
fix: normalize p5 reference links in parameter sections#1284rushdarshan wants to merge 3 commits into
Conversation
ksen0
left a comment
There was a problem hiding this comment.
Thank you for working on this @rushdarshan and the patience in waiting for review.
I tested:
- The original test case with /text page leading to rectMode. Here, the fix works. ✅
- Another example https://p5js.org/reference/p5/lightness/ which currently links correctly to https://p5js.org/reference/p5/p5.Color/ but with the PR fix, it goes to http://localhost:4321/reference/p5.Color/ (missing
p5/❌ )
Similarly with http://localhost:4321/reference/p5/ambientLight/ - the current behavior links p5.Color correctly in both description and parameters. In this PR, the description link works, but parameters link is broken. They use the name relative path in the jsdocs, so this is something the logic should hopefully handle.
If you're still interested in working on this, could you update the logic to cover both cases? Additionally, please check if it is possible to add a unit test for each case so the check can be automated.
Thanks again! and if you're not able to work on this anymore, please also let us know so it can be reassigned.
|
Thanks for the detailed review — I’ve updated the logic to handle both link formats in parameter sections:
I also added unit tests for both cases (including lightness/ambientLight style p5.Color links in parameters) so this is now covered automatically. Could you please re-review when you get a chance? |
|
@Nwakaego-Ego @doradocodes does one of you have capacity to review this please? Would be nice to get it in to both branches if applicable before the website switch, if possible! thanks all |
|
Hi @rushdarshan, thank you for this PR. I tested the fix locally on your branch and can confirm: The rectMode() link in the Parameters section of the text() reference page now navigates correctly to /reference/p5/rectMode/. The p5.Color link in the Parameters section of the lightness() reference page now navigates correctly to /reference/p5/p5.Color/. I also ran npm test and all three tests in referenceLinkUtils.test.ts passed. Moving the function to its own utility file makes it easier to test and reuse. The logic correctly handles both slash notation and dot notation link formats. |
There was a problem hiding this comment.
@rushdarshan moving this out of the ReferenceItemLayout.astro file makes sense, but I think it should be moved into the src/utils and rename the file to referenceLink.ts to match the existing pattern of handling util files.
|
Hi @rushdarshan are you still working on this one to address the review comments? Please let us know in the next ~week (by July 3) thanks in advance! |
94cfadb to
1645038
Compare
Fixes processing#674 The regex in normalizeP5ReferenceLinks() was capturing the 'p5' prefix in the capture group, causing duplicate 'p5' in URLs. Before: #/p5/rectMode -> /reference/p5/p5/rectMode/ (broken) After: #/p5/rectMode -> /reference/p5/rectMode/ (fixed) Updated regex to exclude 'p5' from capture group while still handling both dot notation (#/p5.Element) and slash notation (#/p5/rectMode) patterns.
1645038 to
4ff0df2
Compare
Fixes #674
The regex in normalizeP5ReferenceLinks() was capturing the 'p5' prefix in the capture group, causing duplicate 'p5' in URLs.
Before: #/p5/rectMode -> /reference/p5/p5/rectMode/ (broken)
After: #/p5/rectMode -> /reference/p5/rectMode/ (fixed)
Updated regex to exclude 'p5' from capture group while still handling both dot notation (#/p5.Element) and slash notation (#/p5/rectMode) patterns.