Skip to content
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

Add Home link placement in the header for better accessibility #76

Merged
merged 9 commits into from
Oct 23, 2024

Conversation

Chirag-S-Kotian
Copy link
Contributor

Description

This PR updates the Home link functionality and layout as per the review feedback. The goal is to ensure the Home link is easily accessible and seamlessly integrated into the header, improving the overall user experience.

Updates:

  • Improved Home link placement in the header for better accessibility and consistency.
  • Adjusted spacing and layout to align with UI best practices.
  • Ensured that the Home link redirects to the root page ("/") without any issues.

Feedback Incorporated:

  • Refined the link placement to make it more prominent and user-friendly.
  • Enhanced visual hierarchy in the header to maintain clarity and ease of use.

Screenshots

image

Checklist:

  • Verified the Home link navigation works as expected.
  • Ensured no visual or functional issues with the layout changes.
  • Incorporated feedback from previous PR review.

@Chirag-S-Kotian Chirag-S-Kotian requested a review from a team as a code owner October 22, 2024 03:51
Copy link

vercel bot commented Oct 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
semaphore-boilerplate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 1:03pm

@Chirag-S-Kotian
Copy link
Contributor Author

review please author

Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Chirag-S-Kotian thank you very much!

@Chirag-S-Kotian
Copy link
Contributor Author

IF YOU NEED ANY OTHER HELP PLEASE LET ME KNOW.

@vplasencia
Copy link
Member

vplasencia commented Oct 22, 2024

Hey @Chirag-S-Kotian I left a comment to improve UX. Feel free to push the new commit here without closing the Pull Request.

@Chirag-S-Kotian
Copy link
Contributor Author

i fixed the link problem.

Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you run yarn prettier and push the changes?

apps/web-app/src/components/PageContainer.tsx Outdated Show resolved Hide resolved
Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I see that yarn prettier:write worked well. Could you remove the package-lock.json and yarn.lock files. Any package version should be updated here.

@@ -16,10 +16,10 @@
"@emotion/react": "^11.11.3",
"@emotion/styled": "^11.11.0",
"@semaphore-protocol/core": "4.3.1",
"@semaphore-protocol/data": "4.3.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to update these versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please tell me how to remove package.json and yarn.lock , should I delete from my repo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey!

You can delete the package-lock.json file from your repo.

Regarding the yarn.lock file, if you keep the current versions of all the packages, everything should be fine and you won't need to remove this file. To keep the current versions of all the packages you need to have this: "@semaphore-protocol/data": "4.3.1" instead of this: "@semaphore-protocol/data": "^4.4.1" and "next": "14.1.0" instead of "next": "^14.2.15" in the package.json file.

Please let me know if you have any other questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay i removed both and updated package.json file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Great! Thank you! Please, could you create an empty yarn.lock file in the root of the project and run the command yarn and push the changes? The yarn.lock file should still be in the repo but this PR shouldn't modify it.

After that, I think the PR is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did it anyway and pushed same yarn.lock which is present in boilerplate repo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! The new yarn.lock file is almost correct. If you run yarn, it will be updated and you can push the change. The change will be just adding a line at the end of the file.

The yarn.lock file should not be part of these files after you push the change: https://github.com/semaphore-protocol/boilerplate/pull/76/files

Right now it's there: https://github.com/semaphore-protocol/boilerplate/pull/76/files#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2de

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay changes are complete.

Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thank you @Chirag-S-Kotian!

I added two suggestions. Please push the changes when you update those versions and run yarn.

apps/web-app/package.json Outdated Show resolved Hide resolved
apps/web-app/package.json Outdated Show resolved Hide resolved
Copy link
Member

@vplasencia vplasencia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Chirag-S-Kotian thank you very much!

@vplasencia vplasencia merged commit f8dd530 into semaphore-protocol:main Oct 23, 2024
2 checks passed
Copy link

gitpoap-bot bot commented Oct 23, 2024

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2024 Semaphore Contributor:

GitPOAP: 2024 Semaphore Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@Chirag-S-Kotian
Copy link
Contributor Author

thank you for your time, if you have anything other issue please let me know.

@vplasencia vplasencia linked an issue Oct 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to go to the first page of the app
2 participants