-
Notifications
You must be signed in to change notification settings - Fork 177
feat: adds typscript and node v20 support #350
Conversation
Co-authored-by: Tristan Windham <tristanwindham@gmail.com> Co-authored-by: Dylan Bathurst <dylanbathurst@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. No major concerns. A couple nits and questions and there are some conflicts to resolve as well. Thanks for doing this!
vite.config.ts
Outdated
|
||
// https://vitejs.dev/config/ | ||
export default defineConfig({ | ||
base: "/caravan/#", | ||
base: "/caravan/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a memory of there being issues when we moved away from the hash based routing, but I can't remember what it was. Maybe something to do with direct links to pages on the deployed version of the app. Do you remember why this change was necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this had to do with the github hosted page not working properly, was added back here --> #323
i believe the tldr is ... if you hit caravan site directly and navigated around, everything was fine - but if you tried to hit a specific page (not the base site), it'd 404.
@@ -0,0 +1 @@ | |||
declare module "unchained-wallets"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔜
@@ -15,7 +15,7 @@ import { | |||
braidDetailsToWalletConfig, | |||
} from "unchained-wallets"; | |||
import { Box, Table, TableBody, TableRow, TableCell } from "@mui/material"; | |||
import { externalLink } from "../utils"; | |||
import { externalLink } from "utils/ExternalLink"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
@@ -1,6 +1,9 @@ | |||
import React from "react"; | |||
|
|||
export default (url, text) => ( | |||
export const externalLink = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be capitalized/Pascal case since it's a component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. It should be a normal component but I see that the way it's being used elsewhere isn't jsx style. We don't need to address this now.
Just need to revert the removal of the hash routing (or explicit confirmation that it won't break things for published versions of the site) and then I think we're good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Description
Does this PR introduce a breaking change?
Does this PR fix an open issue?