Corrected unordered list with list items - #100
Conversation
|
@mahataanwesha is attempting to deploy a commit to the Sahil's projects Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @mahataanwesha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the structural integrity and accessibility of the application's main navigation. By transitioning from a non-semantic Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the navigation menu to use a semantic <ul> with <li> elements, which is a great improvement for accessibility and SEO. My review includes a suggestion to further enhance the code by abstracting the navigation links into a configuration array. This will make the navigation component more maintainable and easier to update in the future.
| <ul className="hidden lg:flex lg:ml-10 xl:ml-16 lg:items-center lg:justify-center lg:space-x-8 xl:space-x-16"> | ||
| <li> | ||
| <Link | ||
| to="about" | ||
| smooth={true} | ||
| duration={500} | ||
| offset={-50} | ||
| className="cursor-pointer text-base font-medium text-gray-900 dark:text-gray-100 transition-all duration-200 rounded focus:outline-none font-pj hover:text-opacity-50 focus:ring-1 focus:ring-gray-900 dark:focus:ring-gray-100 focus:ring-offset-2" | ||
| > | ||
| About Travel-Book | ||
| </Link> | ||
| </li> | ||
|
|
||
| <li> | ||
| <Link | ||
| to="how-it-works" | ||
| smooth={true} | ||
| duration={500} | ||
| offset={-50} | ||
| className="cursor-pointer text-base font-medium text-gray-900 dark:text-gray-100 transition-all duration-200 rounded focus:outline-none font-pj hover:text-opacity-50 focus:ring-1 focus:ring-gray-900 dark:focus:ring-gray-100 focus:ring-offset-2" | ||
| > | ||
| How does it work? | ||
| </Link> | ||
| </li> | ||
|
|
||
| <li> | ||
| <Link | ||
| to="services" | ||
| smooth={true} | ||
| duration={500} | ||
| offset={-50} | ||
| className="cursor-pointer text-base font-medium text-gray-900 dark:text-gray-100 transition-all duration-200 rounded focus:outline-none font-pj hover:text-opacity-50 focus:ring-1 focus:ring-gray-900 dark:focus:ring-gray-100 focus:ring-offset-2" | ||
| > | ||
| Our Services | ||
| </Link> | ||
| </li> | ||
|
|
||
| <li> | ||
| <a | ||
| href="/contributors" | ||
| className="text-base font-medium text-gray-900 dark:text-gray-100 transition-all duration-200 rounded focus:outline-none font-pj hover:text-opacity-50 focus:ring-1 focus:ring-gray-900 dark:focus:ring-gray-100 focus:ring-offset-2" | ||
| > | ||
| Contributors | ||
| </a> | ||
| </li> | ||
| </ul> |
There was a problem hiding this comment.
While the change to use <ul> and <li> is great for semantics, the navigation items are hardcoded and repetitive. This can be improved for better maintainability by creating a configuration array for the navigation links and rendering them dynamically using .map(). This makes it much easier to add, remove, or reorder links in the future.
I've included a self-contained suggestion, but for better performance, the navLinks array and linkClassName constant should be defined outside the component.
<ul className="hidden lg:flex lg:ml-10 xl:ml-16 lg:items-center lg:justify-center lg:space-x-8 xl:space-x-16">
{(() => {
const navLinks = [
{ type: 'Link', to: 'about', label: 'About Travel-Book' },
{ type: 'Link', to: 'how-it-works', label: 'How does it work?' },
{ type: 'Link', to: 'services', label: 'Our Services' },
{ type: 'a', href: '/contributors', label: 'Contributors' },
];
const linkClassName = "text-base font-medium text-gray-900 dark:text-gray-100 transition-all duration-200 rounded focus:outline-none font-pj hover:text-opacity-50 focus:ring-1 focus:ring-gray-900 dark:focus:ring-gray-100 focus:ring-offset-2";
return navLinks.map((link) => (
<li key={link.label}>
{link.type === 'Link' ? (
<Link
to={link.to}
smooth={true}
duration={500}
offset={-50}
className={`cursor-pointer ${linkClassName}`}
>
{link.label}
</Link>
) : (
<a
href={link.href}
className={linkClassName}
>
{link.label}
</a>
)}
</li>
));
})()}
</ul>
|
Hi @mahataanwesha, You made changes into various files instead of only Hero.jsx right? |
No for this commit I have changed only hero.jsx |
2 participants
The navigation menu in
src/pages/hero/Hero.jsx
has been updated to use a semantic unordered list (
) with list items (- ) instead of a div. This ensures compliance with W3C HTML5 standards and improves accessibility tree mapping and semantic SEO.
Changes made:
File:
src/pages/hero/Hero.jsx
Action: Replaced the div container for the desktop navigation menu with
ul
and wrapped each Link/a tag in an
li
element.
Result: The navigation structure is now:
jsx
...
This corrects the structure where anchor tags (or Link components rendering anchors) were direct children of the container, providing a proper list structure for screen readers and search engines