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

fix: 1135 wiring up veda config #1320

Merged

Conversation

snmln
Copy link
Contributor

@snmln snmln commented Dec 11, 2024

@AliceR I am getting the menu items wired up.

Copy link

netlify bot commented Dec 11, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 886912e
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/675c8caf4fabf60008558552
😎 Deploy Preview https://deploy-preview-1320--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@snmln snmln changed the base branch from main to 1135-Refactor-Layout-Components-Footer December 11, 2024 22:20
Comment on lines 49 to 78
const createNavElement = (navItems, linkClasses) => {
//removing 'dropdown' items from array
let cleanedNavItems = navItems.filter((a) => {
if (a.type !== 'dropdown') {
return a;
}
});

return cleanedNavItems.map((item) => {
switch (item.type) {
case NavItemType.ACTION:
return <NavItemCTA item={item} />;

case NavItemType.EXTERNAL_LINK:
return (
<a className={linkClasses} href={item.to} key={item.id}>
{item.title}
</a>
);
case NavItemType.INTERNAL_LINK:
return (
<a className={linkClasses} href={item.to} key={item.id}>
{item.title}
</a>
);

default:
return <></>;
}
});
Copy link
Contributor Author

@snmln snmln Dec 11, 2024

Choose a reason for hiding this comment

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

I know we have similar functionality from the header work here: https://github.com/NASA-IMPACT/veda-ui/blob/main/app/scripts/components/common/page-header/nav/create-dynamic-nav-menu-list.tsx

The issue is that the header styling is implicit to this function. We should reuse this functionality but expand it to accept different stylings. I need to look into this a little more to wrap my head around it better. But based on my current understanding that seems like a decent amount of work to accomplish.

My opinion (with my current understanding) is that we note it and create a dedicated clean up ticket to integrate that styling functionality since this will bleed into the header work that was just created.

@AliceR With your familiarity with the functionality what are your thoughts?

Copy link
Collaborator

@sandrahoang686 sandrahoang686 Dec 12, 2024

Choose a reason for hiding this comment

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

you could move the nav directory with createDynamicNavMenuList et al outside of header to be accessible to header&footer and still use the nav-item-link and CTA components like createDynamicNavMenuList already does but make it take in the className now as a param and pass it down as a prop to the nav link components. I dont think footer will need to worry about dropdowns. For CTA, you could make the style a prop as well but default it.

That doesn't have to be done here though, feel free to leave an @TODO note in the code noting the need to consolidate and create a ticket for it 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet I have gone ahead and created a new issue: #1323 for this.

@AliceR AliceR added the design system change https://github.com/NASA-IMPACT/veda-ui/blob/main/docs/adr/003-design-system-change.md label Dec 12, 2024
@snmln snmln changed the title WIP - 1135 wiring up veda config fix: 1135 wiring up veda config Dec 12, 2024
@snmln snmln marked this pull request as ready for review December 12, 2024 21:09
@snmln snmln requested a review from AliceR December 13, 2024 16:04
@AliceR AliceR merged commit 8c3c769 into 1135-Refactor-Layout-Components-Footer Dec 16, 2024
9 of 10 checks passed
@AliceR AliceR deleted the 1135-wiring-up-veda-config branch December 16, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design system change https://github.com/NASA-IMPACT/veda-ui/blob/main/docs/adr/003-design-system-change.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants