open-saas icon indicating copy to clipboard operation
open-saas copied to clipboard

Replacing a tags with link tags in AppNavBar

Open npenza opened this issue 1 year ago • 3 comments

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:

  • [ ] Update e2e tests: If you changed the /template/app, then make sure to do any neccessary updates to /template/e2e-tests also.
  • [x] Update demo app: If you changed the /template/app, then make sure to do any neccessary updates to /opensaas-sh/app_diff also. Check /opensaas-sh/README.md for details.
  • [ ] Update docs: If needed, update the /opensaas-sh/blog/src/content/docs.

npenza avatar Aug 30 '24 12:08 npenza

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' },
];

vincanger avatar Sep 02 '24 08:09 vincanger

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() },
];

npenza avatar Sep 02 '24 09:09 npenza

@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.

vincanger avatar Sep 02 '24 10:09 vincanger