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

Replacing a tags with link tags in AppNavBar #264

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

npenza
Copy link

@npenza npenza commented Aug 30, 2024

Description

Describe your PR! If it fixes specific issue, mention it with "Fixes # (issue)".

Fixes #254. Replaces anchor tags with link tags to prevent full page reloads when navigating the app navbar on both desktop and mobile.

I had to restructure the navigation array as outbound links can't use the link tags, and must use anchor tags. They are rendered after mapping through all the prior navigation items. Happy to discuss/implement a better solution.

Contributor Checklist

Make sure to do the following steps if they are applicable to your PR:

@vincanger
Copy link
Collaborator

vincanger commented Sep 2, 2024

Thanks for the contribution @npenza!

Two things to consider:

  1. I'm wondering why we can't use the Link component that Wasp exports since most (all?) of these links are routes defined in the main.wasp file and could benefit from its typesafety feature. Did you try it out and run into any issues?
  2. We also need to update the landing page, as it has its own, separate "nav bar" in src/landing-page/components/Header.tsx.

Edit:

Maybe this might be a nice solution, or am I forcing the use of Wasp's Link component here? WDYT @infomiho ?

import { Link, routes, type Routes } from 'wasp/client/router';
//...

interface NavigationItem {
  name: string
  href: Routes['to']
}
const navigation: NavigationItem[] = [
  { name: 'AI Scheduler (Demo App)', href: '/demo-app'}, // because we typed it, we get autocompleted routes here based on our main.wasp file
  { name: 'File Upload (AWS S3)', href: '/file-upload' },
  { name: 'Pricing', href: '/pricing' },
];

@npenza
Copy link
Author

npenza commented Sep 2, 2024

Thanks @vincanger. We definitely can use Wasp's Link component.

Before I make the changes, can you please clarify the differences between .build() and .to for the route objects? E.g this works:

const navigation = [
  { name: 'AI Scheduler (Demo App)', href: routes.DemoAppRoute.to },
  { name: 'File Upload (AWS S3)', href: routes.FileUploadRoute.to },
  { name: 'Pricing', href: routes.PricingPageRoute.to },
];

But this causes a ts error 'Type 'string' is not assignable to type' when mapping through navigation.

const navigation = [
  { name: 'AI Scheduler (Demo App)', href: routes.DemoAppRoute.build() },
  { name: 'File Upload (AWS S3)', href: routes.FileUploadRoute.build() },
  { name: 'Pricing', href: routes.PricingPageRoute.build() },
];

@vincanger
Copy link
Collaborator

@npenza check out the proposed solution I gave above. The Wasp Link component expects a type which is a union of the route strings, so it functions more like an enum. That's why it won’t accept a general string, which is the error you referenced. The reason the build method throws this error is because it’s typed to return a string, which doesn’t match the union of route strings.

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 navigation in template use Link tags rather then A tags.
2 participants