-
-
Notifications
You must be signed in to change notification settings - Fork 221
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: link-redirect-fix #1046
fix: link-redirect-fix #1046
Conversation
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
===========================================
- Coverage 100.00% 99.47% -0.53%
===========================================
Files 10 10
Lines 373 380 +7
Branches 94 97 +3
===========================================
+ Hits 373 378 +5
- Misses 0 2 +2 ☔ View full report in Codecov by Sentry. |
What is the issue associated with this PR? #1308 does not exists. Thanks. |
Contributor mentioned the wrong issue no. before, I've edited to the real one : ) |
@DarhkVoyd Can you please help reviewing this PR? |
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.
Hey, thanks for the PR. Great idea to refactor the logic into a cleaner snippet. I have some suggestions before we merge this.
- The prop name
markdownFile
is outdated and is no longer clear, I think we should update it to be more descriptive and clear. - In
DocsHelpProps
, let's keep the prop optional but narrow the prop type fromstring
to clear descriptive String Literal Types such astsx | _indexmd | indexmd
, better for DX with LSP suggestions. - I noticed you missed a few files, so can you please look in the codebase and ensure you cover all the instances with the updates consistently. For instance, edit button at
http://localhost:3000/docs
is still broken.
All in all good work with the proposed changes and implementation!
I have made all the changes requested. Waiting for your feedback @DarhkVoyd. |
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.
Props are conventionally camelCase, while PascalCase is typically for classes and components.
I have changed the variable name into camel case. Waiting for feedback @DarhkVoyd |
@DarhkVoyd Waiting for your feedback!!!! |
@Yashwanth1906 Could you please add tests to cover these two lines? They appear to be missing. Thank you : ) https://github.com/json-schema-org/website/pull/1046/checks?check_run_id=32220879357 |
Hey @Yashwanth1906, right now the lockfile is changing in CI, maybe because there are some changes in the package versions, but the lockfile was not updated for it. You can delete the lockfile and then reinstall it to generate a new one and push the updated lockfile. Most of the workflows are failing because of this. |
@aialok I did that but it is still failing because I added a bunch of code in the docshelp.cy.tsx to cover those two lines. I think there is the issue. Can u help me figure out the error in that test file docshelp.cy.tsx. |
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.
Good work! Almost there. However, there are a few minor adjustments and fixes I suggest:
- The
/tools
endpoint should have thefileRenderType
property set totsx
. - Since the prop name has been updated to
fileRenderType
, it’s advisable to update the passed argument variable name to match the change. Instead of the currentmarkdownFile
.
@
@aialok I can't figure out the failing tests. |
Did you try https://nextjs.org/docs/messages/next-router-not-mounted? |
@Yashwanth1906 Any update on this one? |
I tried the above thing u suggested but it is not working. So, Im figuring out myself. Im new in writing tests in javascript. So, Im learning and trying to implement on the go. |
What kind of change does this PR introduce?
Issue Number:
Screenshots/videos:
If relevant, did you update the documentation?
Summary
This pull request addresses an issue related to incorrect URL redirection in the JSON Schema website. Users were being redirected to incorrect or unintended pages when clicking on certain links.
Changes Made:
Corrected Redirecting URLs: Updated the wrong redirecting URLs in various components to ensure they point to the correct destinations. This change enhances navigation and provides users with access to the intended content.
Refactored JSX Components: Adjusted the formatting of JSX components to comply with the project's style guide. Specifically, I ensured that there were proper spaces before closing brackets in the JSX tags, addressing the errors raised by the reviewers.
Code Consistency: Ensured consistency in the codebase by adhering to Prettier and ESLint rules, which included correcting formatting issues to enhance code readability and maintainability.
These changes improve user experience by ensuring that links function correctly, reducing frustration caused by misdirected navigation.
Does this PR introduce a breaking change?