-
-
Notifications
You must be signed in to change notification settings - Fork 20
feat: website #69
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
feat: website #69
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit de68e15:
|
|
@dai-shi Is the CI enabled for new workflows, we'd like to add the github pages. |
CI is enabled. You mean another workflow for github pages? btw, if we go with github pages, we can apply for proxy-memoize.js.org. |
Yes
That'd be good actually, but how about the github one? As far as I know they used to give free domains for github pages. |
|
Can you check the workflow file? Doesn't seem to work?
I didn't know about it. Never used before. |
Let's use it and see, we'll let you know.
|
| export default defineConfig({ | ||
| // setup for github pages | ||
| site: 'https://pheno-agency.github.io', | ||
| base: '/proxy-memoize', |
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.
@Sajad-Sharhani I think the base should be /website, it's the directory that the site lives in. And please add the github workflow that astro recommends.
website/src/components/Navbar.astro
Outdated
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| </head> |
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.
@Morteza-Mazrae I guess you need to remove the whole head here cause we already have it in the layout.
website/src/components/Sidebar.astro
Outdated
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| </head> |
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.
@Morteza-Mazrae also here
website/src/pages/index.astro
Outdated
| @@ -16,6 +17,9 @@ const docs = await Astro.glob('../../../docs/**/*.md'); | |||
| <title>Proxy Memoize</title> | |||
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.
@Morteza-Mazrae move these stuff to the layout (the head section, they all should be in one place)
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| </head> |
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.
@Jawad-sawari this head thing should be removed, we don't need them, it's not logical.
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.
@Morteza-Mazrae I don't think in astro components we bring and
only the elements we need, in case of this file, the
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.
@Aslemammad I removed the whole extra head tags
|
Hey @dai-shi we've been able to finish the website, now you can take a look and see! Let us know, it's been a fun experience. |
|
https://pheno-agency.github.io/proxy-memoize/ Here's the lighthouse score: We've been able to keep most of the site static and keep some level of interactivity at the same time so we can achieve such a lighthouse result. Only one component is written in react and all of the rest are in astro (simple html with some script tags) |
dai-shi
left a comment
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.
Thanks for your work!!!
Some comments added. I would like to see it deployed on GitHub pages?
.github/workflows/deploy.yml
Outdated
| # Trigger the workflow every time you push to the `main` branch | ||
| # Using a different branch name? Replace `main` with your branch’s name | ||
| push: | ||
| branches: [feat/website] |
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.
Does this work now in this branch?
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, but since it's a fork, it pushes in our repo, when merged, it should be changed to main. You can see the link here for instance https://pheno-agency.github.io/proxy-memoize/
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.
Hm, okay, let's change it to main in this PR and we will see.
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.
sure, cc @Sajad-Sharhani
.pnpm-debug.log
Outdated
| @@ -0,0 +1,23 @@ | |||
| { | |||
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.
Please remove this file.
.vscode/settings.json
Outdated
| @@ -0,0 +1,2 @@ | |||
| { | |||
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.
Please remove this file.
Co-authored-by: Daishi Kato <[email protected]>
|
Can you please check the CI error? |
dai-shi
left a comment
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.
Thanks for your wonderful work! Let's merge.
|
Hi, I'm trying to deploy it as I'm not sure how it's caused, but if you guys know something, it's helpful. |
|
As I see it's an issue with github pages and not proxy memoize, am I right? |
|
Can u first deploy it on github pages subdomain? |
Yes, it was perfectly fine!
Yeah, seems like so... |
I think you'd be able to issue a ticket to the github support so they know the issue (as others also had it), otherwise, I guess we need to go the plain github pages way or buy our own domain! or move things to vercel (which takes time, since we need to change the assets base from /proxy-memoize/ to / in astro) |
|
fwiw, I never had such an issue with |
|
maybe it's because of the CNAME file? |
|
The custom domain issue is resolved, but now I face the |
|
@Sajad-Sharhani Hey Sajad, Daishi has been able to fix the custom domain issue, can you change the assets paths and fix the issue? Here's the new url https://proxy-memoize.js.org/ |

The Proxy Memoize website. Built with Astro, unocss and mostly vanilla javascript.