-
Notifications
You must be signed in to change notification settings - Fork 137
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
Migrate FDC3 Workbench to Vite #894
Conversation
✅ Deploy Preview for lambent-kulfi-cf51a7 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Looks good to me! Passing Title bar image is broken when there's no trailing slash on the URL, got a fix for that in another PR. Needs main merging into PR. Also es-lint plugins need updating for TypeScript version. Looks like you can also remove the |
8e3dca6
to
9641ae0
Compare
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.
lgtm
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 works great for me - a few questions on asset paths that I'm still looking into
@@ -2,19 +2,20 @@ | |||
<html lang="en"> | |||
<head> | |||
<meta charset="utf-8" /> | |||
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" /> | |||
<link rel="icon" href="/favicon.ico" /> |
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 willresolve to https://fdc3.finos.org/ when deployed which is not right, think it might need to be:
<link rel="icon" href="/favicon.ico" /> | |
<link rel="icon" href="favicon.ico" /> |
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.
but then it appears to be fine in the preview at https://deploy-preview-894--lambent-kulfi-cf51a7.netlify.app/toolbox/fdc3-workbench/
where it requests it from: https://deploy-preview-894--lambent-kulfi-cf51a7.netlify.app/toolbox/fdc3-workbench/favicon.ico correctly.
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.
Yes, these should magically work because of...
base: "/toolbox/fdc3-workbench",
<link rel="apple-touch-icon" href="/fdc3-icon-192.png" /> | ||
<link rel="manifest" href="/manifest.json" /> |
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.
<link rel="apple-touch-icon" href="/fdc3-icon-192.png" /> | |
<link rel="manifest" href="/manifest.json" /> | |
<link rel="apple-touch-icon" href="fdc3-icon-192.png" /> | |
<link rel="manifest" href="manifest.json" /> |
See previous comment
<title>FDC3 Workbench</title> | ||
</head> | ||
<body> | ||
<noscript>You need to enable JavaScript to run this app.</noscript> | ||
<div id="root"></div> | ||
<script type="module" src="/src/index.tsx"></script> |
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.
Not sure about this oath either... remove the preceding /
?
@@ -79,7 +79,7 @@ export const Header = (props: { fdc3Available: boolean }) => { | |||
<Toolbar className={classes.toolbar}> | |||
<div> | |||
<Typography variant="h3" color="inherit" className={classes.fdc3}> | |||
<img src="./fdc3-logo.svg" className={classes.headerCube} /> | |||
<img src={`${import.meta.env.BASE_URL}/fdc3-logo.svg`} className={classes.headerCube} /> |
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.
might get away with just fdc3-logo.svg
here... although that requires the URL used to access to have a trailing / - I've got another hacky fix that just puts that / on in the index page if its not there...
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 created a new version of this PR: And got this preview: https://deploy-preview-923--fdc3.netlify.app/toolbox/fdc3-workbench/ ..which looks correct to my untrained eye? wdyt? |
LGTM 👏 |
Superseded by #923 |
Initial work to migrate FDC3 Workbench to Vite
Builds on top of #859