-
-
Notifications
You must be signed in to change notification settings - Fork 26
Update the nav elements active status using Pathname #24
Update the nav elements active status using Pathname #24
Conversation
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.
Thank you for the contribution 👍
Why is there "use client"
everywhere?
@eddiejaoude on removing the |
Please go through and check your changes, "use client" should not be required. I would recommend checking your local node version as this might be causing the problem. |
@eddiejaoude please review and let me know if any changes required. |
@Muhammad-Owais-Warsi please recheck each of your changes and make sure you are happy with them, because there are so many unnecessary changes |
@eddiejaoude to me it seems to be fine. |
I will comment a few examples in line |
src/app/repo/status/page.js
Outdated
@@ -195,7 +196,7 @@ const repo = { | |||
subscribers_count: 54, | |||
}; | |||
|
|||
const results = checks(repo); | |||
const results = checks(repo); |
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.
I don't think this change is required?
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.
Thank you for making further changes, but more are still required. I have left some example inline comments
Please recheck that all changes in your PR are changes you are happy with, such as extra line changes resulting in files being included in your PR that should not be
Sorry for the inconvenience, I'll recheck and let you know. |
@eddiejaoude Please review , I don't think anything should cause any problem now |
Thank you for your contribution. As there were still unnecessary changes this PR could not be merge, and now as there are conflicts also, I have made a PR to solve this issue #43, so I will close this PR. Hopefully my PR will highlight the changes required, for example the existing condition was the main change only required |
Thanks @eddiejaoude , will try to improve next time |
Thanks @Muhammad-Owais-Warsi for the collaboration |
Fixes Issue Closes #17
Changes proposed
usePathname
to get the current path.Check List (Check all the applicable boxes)
Screenshots