Skip to content

Routing Layout Changed -> Header.astro#20

Open
Vishalk91-4 wants to merge 6 commits intoEshaanAgg:mainfrom
Vishalk91-4:routingLayout
Open

Routing Layout Changed -> Header.astro#20
Vishalk91-4 wants to merge 6 commits intoEshaanAgg:mainfrom
Vishalk91-4:routingLayout

Conversation

@Vishalk91-4
Copy link
Copy Markdown

Previously we were using ul tags, now I've changed it to navitems, routing becomes simpler
Design remains the same

@EshaanAgg, could you please review this pull request

Copy link
Copy Markdown
Owner

@EshaanAgg EshaanAgg left a comment

Choose a reason for hiding this comment

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

This is great PR which has it's heart in the right place. But you need to shift the CSS styles to the correct components logically, instead of dumping them all in one.

Comment thread src/components/Header.astro Outdated
Comment thread src/components/Header.astro Outdated
Comment thread src/components/Header.astro Outdated
Comment thread src/components/Header.astro Outdated
@Vishalk91-4
Copy link
Copy Markdown
Author

Vishalk91-4 commented Oct 17, 2023

@EshaanAgg I've updated it, so that the current layout doesn't break, could you please review it

@Vishalk91-4
Copy link
Copy Markdown
Author

@EshaanAgg, resolved conflicts

@EshaanAgg
Copy link
Copy Markdown
Owner

Please address the comments I have added earlier. Also attach a video of the the header showing if it is responsive or not.

@Vishalk91-4
Copy link
Copy Markdown
Author

Vishalk91-4 commented Nov 5, 2023

Resolved Comments

LFX.Mentorship.and.62.more.pages.-.Work.-.Microsoft.Edge.2023-11-06.00-26-04.mp4

@EshaanAgg, resolved them all

Copy link
Copy Markdown
Owner

@EshaanAgg EshaanAgg left a comment

Choose a reason for hiding this comment

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

Can you please explain the rationale behind changing the content of the navbar?

Comment on lines +13 to +19
path: '/org',
label: 'Organizations',
},
{
path: '/organization/PastProjectCard',
label: 'Projects',
},
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These links do not exist in the site.

Comment on lines +24 to +29
{/*
const navItems = [
{ name: "Home", link: "/" },
{ name: "GitHub", link: "https://github.com/EshaanAgg/LFXMM-Frontend" },
{ name: "Team", link: "/team" },
];
]; */}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why did you change the navbar content?

</nav>
{/*
<div
class="text-2xl text-white mb-5 sm:mb-0 px-10 py-2 h-36 sm:h-20 sm:py-8 flex pb-4"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove the commented out code.

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.

2 participants