-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Website: Migrate to vitepress #6518
Conversation
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.
Some One larger questions:
- Is it feasible to move any JS code that "does work" into a library, so we can run linters and formatters and such?
- name: HTML Proofer | ||
run: bundle exec htmlproofer ./_site --check-html --check-opengraph --report-missing-names --log-level=:debug --assume-extension --empty-alt-ignore --timeframe=6w --disable-external |
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.
It looks like you're turning off the "check links in the final rendered output" (HTMLProofer
) check, so we'd be relying on the markdown link checker entirely. Is my understanding correct, and is that what we want?
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, since we want to remove all ruby stuff. If you want, I can put in a new action that runs htmlproofer against the built 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.
Hm, I'm sure there's a better option than htmlproofer if we're moving away from Ruby.
.vitepress/config.js
Outdated
const eips = Promise.all(fs.readdirSync('./EIPS/').map(async file => { | ||
let eipContent = fs.readFileSync(`./EIPS/${file}`, 'utf8'); | ||
let eipData = grayMatter(eipContent); | ||
let lastStatusChange = new Date((await git.raw(['blame', `EIPS/${file}`])).split('\n').filter(line => line.match(/status:/gi))?.pop()?.match(/(?<=\s)\d+-\d+-\d+/g)?.pop()); |
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 library automatically add --porcelain
? If not, I'd very much recommend using it, or maybe even --line-porcelain
.
I know we disallow colons in eipw
, but if we change that, we should still expect this tool to work. I'd recommend using /^status:/gi
as the regex in case someone puts something dumb in their 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.
What is --porcelain?
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.
git has two types of commands: plumbing and porcelain. Plumbing commands are low-level and are used to implement the higher level porcelain commands, while porcelain commands are made to interface with a human.
The --porcelain
flag tells porcelain commands to output in a more stable and script friendly format.
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 know we disallow colons in eipw, but if we change that, we should still expect this tool to work. I'd recommend using /^status:/gi as the regex in case someone puts something dumb in their title.
How does that break this line?
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.
How does that break this line?
It might, it might not. I'm not sure 🤣 Those were two unrelated thoughts: use --porcelain
, and separately, tighten the regex.
🛑 |
The commit 67a6a88 (as a parent of a8de4c9) contains errors. |
I like the site a lot. And I don't think Vitepress / HTML is a fair comparison here, probably we are comparing Jekyll vs Vue here.. Anyway, I've redirected eips.ercref.org to https://eips.pandapip1.com/ and will start using it for myself. @Pandapip1 if you are open to it, we could setup eips.ercref.org or find a proper first level domain for it? As much as we like to have (evm) client diversity, I've also been a big fan to support dApp diversity. |
Vitepress is still in beta so we probably shouldn't be relying on it. I also don't think we should be making the site more complicated. I would rather we focus on making it simpler with less javascript. We don't need complex frontend frameworks to show EIPs. |
Very much agreed. I don't know anything about Vue or Vitepress, and I—perhaps naively—assumed this was all executed during the compilation step. Search, filtering, and any tooltips will likely need to be client-side, however. |
It's ok if you feel that this site is not ready to be used for the canonical eips.ethereum.org if there are concerns. I am in support for this site to be its stand alone to support the EIP/ERC ecosystem. |
That's fine, but in that case it should live in a different repo and this PR closed. |
@Pandapip1 are you ok if this is a stand alone site? I think that reduces the friction for your beautifully designed site to be adopted sooner. And we can keep adding features to it. |
This is all executed during the compilation step, minus the searching which is done using a little bit of client-side JS, and some minor styling (tooltips, also with client-side JS). |
Great! That takes care of my trepidation about this. My outstanding concerns:
@lightclient Would I be correct in saying your only remaining objections are:
|
Closing since I've forked these changes to a separate repo. If we want to implement this, the website will be in a seperate repo. https://eip.info is an automatically updated but improved version of these changes. |
There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
It appears I forgot to close. |
Preview (might be out of date): https://eips.pandapip1.com/
Closes #6483
TODO:
Not necessary, nice to have:
Other Notable Changes