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

Refactor Menu Links Using Constants and Map Function for Better Code Structure as per industry standards in src/components/Navbar.tsx #107

Open
yashpandey06 opened this issue Sep 23, 2024 · 5 comments
Assignees

Comments

@yashpandey06
Copy link

Title: Refactor Menu Links Using Constants and Map Function for Better Code Structure

Description:
In the current implementation of the Menu.ItemGroup, the href, target, rel, and other fields are hardcoded directly in the JSX. This approach can lead to redundancy and reduce maintainability, especially as the application scales.

Current Scenerio :
WhatsApp Image 2024-09-23 at 11 23 36

Proposed Solution:
To improve the code quality and follow industry-level best practices, we can:

  1. Store the links (href, target, rel, etc.) in a constant file to centralize these values and make them easier to manage.
  2. Use the map() function to dynamically render Menu.ItemGroup components. This will not only reduce code duplication but also make it more maintainable and scalable.

Benefits:

  • Centralized control of URLs and link-related attributes.
  • Cleaner and more readable JSX code.
  • Easier to update URLs or attributes in one place without modifying multiple components.
  • Increased scalability as the application grows.

Example:

const menuItems = [
  { key: 'about', href: 'https://github.com/accordproject/template-playground/blob/main/README.md', icon: <QuestionOutlined />, label: 'About' },
  { key: 'community', href: 'https://discord.com/invite/Zm99SKhhtA', icon: <UserOutlined />, label: 'Community' },
  { key: 'issues', href: 'https://github.com/accordproject/template-playground/issues', icon: <InfoOutlined />, label: 'Issues' },
  { key: 'documentation', href: 'https://github.com/accordproject/template-engine/blob/main/README.md', icon: <BookOutlined />, label: 'Documentation' },
];

return (
  <Menu.ItemGroup title="Info">
    {menuItems.map(item => (
      <Menu.Item key={item.key}>
        <a href={item.href} target="_blank" rel="noopener noreferrer">
          {item.icon} {item.label}
        </a>
      </Menu.Item>
    ))}
  </Menu.ItemGroup>
);
@Vinyl-Davyl
Copy link
Collaborator

Hey @yashpandey06 👋 Great point out. Really a valid one. I agree!

Do you have intentions with this? If not, surely we would look into this, definitely.

@yashpandey06
Copy link
Author

Hey @yashpandey06 👋 Great point out. Really a valid one. I agree!

Do you have intentions with this? If not, surely we would look into this, definitely.

Yeah , I would like to work on this ☺️.If you have not started yet.

@Vinyl-Davyl
Copy link
Collaborator

Vinyl-Davyl commented Sep 23, 2024

Okay, would leave that to you. Sure you can! @yashpandey06

sure be mindful of the test cases in place for the active links in the tests folder. should incase there are any breaks in the test while at it to be modified.

@yashpandey06
Copy link
Author

I'm not sure why, but when I try to start my Vite server, the screen is completely blank with no errors being thrown. Could you help me with this?

@Vinyl-Davyl

@Vinyl-Davyl
Copy link
Collaborator

Vinyl-Davyl commented Sep 26, 2024

I'm not sure why, but when I try to start my Vite server, the screen is completely blank with no errors being thrown. Could you help me with this?

@Vinyl-Davyl

Hey Yash, I guess you should check your console on the browser, you would probably see something relating to the <Router> or BrowserRouter. So as a fresh start to the application, vite is probably loading up the Mixed ESM and CJS like in the @accordproject/template-engine and more. So it's a cold restart.

I suggest you give it some more time, try clearing your cache or deleting and install your node modules again if possibe
npm cache clean --force rm -rf node_modules npm install. Let me know when you make progress with this

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

No branches or pull requests

2 participants