-
Notifications
You must be signed in to change notification settings - Fork 41
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 V2 #389
base: main
Are you sure you want to change the base?
Website V2 #389
Conversation
9efd818
to
9dd281c
Compare
a65f6be
to
2bbd454
Compare
✅ Deploy preview ready! 🌾
To edit notification comments on pull requests, go to your Netlify site configuration. |
2bbd454
to
26c1b58
Compare
5748161
to
24076fc
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.
Great work on this Alex, the new site is amazing and the share feature on the playground is awesome for being able to share reproducable snippets. Most of my feedback is small a tiny bit of will need a bit of discussion I think and I also left some docs fixes but I don't know if we want to address those in this pr or not.
Below is some general feedback that did not really relate to any file in particular or I couldn't find where it would need to be changed.
Blog
- If you take a look here you'll notice we have an
include ModuleName
inline, I feel like the style is slightly off maybe we add a tiny bit more padding, slight border or make the background a tiny bit darker? I think discord does a really good job with the style of these for inspiration. - There seems to be some overflow on mobile in the description of the docs, I really don't know the best way for this to be fixed, in the example below example we could certainly reduce the padding around the param column but that doesn't seem like a full fix, maybe we should open an issue for this though if we can't come up with something better as I would argue it's minor?
- Can we use https://docs.astro.build/en/recipes/rss/ to add the blogs rss feed back.
- I think it would be fine if we opened an issue and handled this in a separate pr.
changeView(0); | ||
</script> | ||
|
||
<div class="flex flex-col relative h-full w-full"> |
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.
In light mode this stays dark, I found the contrast between the light background and this to be way to much making it really hard to read. (I noticed we might have the same issue on codeblocks in the docs as well), I don't know the best fix but I think if we just made the purple a bit more faded in lightmode (maybe we head more towards a lavender) it would help a lot.
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.
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.
Where is this?
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 file specifically is the home page hero with the code examples, but I was asking if we should just change all rendered code blocks in light mode to be a light theme
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.
Just a couple of accessibility notes I forgot in the first review as well, the mobile nav button also needs an aria label.
138baaa
to
4b2bcc0
Compare
@@ -34,9 +34,11 @@ git pull --recurse-submodules | |||
|
|||
To make a change to a document, edit the corresponding Markdown file in [src](src). The file path matches the URL path after `/docs`, but if you have trouble finding the page you're looking for, you can click the "Edit on GitHub" button at the top of page on the website. | |||
|
|||
Please note that standard library documentation is auto-generated from our standard library source with our documentation tool `grain doc`. If you'd like to make an edit to the standard library docs please do so in the corresponding source file in the main compiler monorepo found [here](https://github.com/grain-lang/grain/tree/main/stdlib). After editing the source file, you can run `grain doc stdlib -o stdlib --current-version=$(grain -v)` from the project root directory to generate the `.md` docs. The changes will be reflected on the website the next time we deploy changes for the next release! |
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.
Could offer here Jake's new npm run stdlib doc
.
site: "https://grain-lang.org", | ||
vite: { | ||
// We want to include wasm files as raw data and then we glob for their urls | ||
assetsInclude: ["**/*.wasm"], |
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 think this'll break with the new object files.
<a href="#"> | ||
<Chip> | ||
<span class="font-medium mr-1">New release!</span> Grain | ||
v0.6 - Emmer | ||
<ExternalIcon class="text-color-dim w-3 h-3 ml-1.5" /> | ||
</Chip> | ||
</a> |
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.
Can we make this a real link to that blog post? We'll probably update this to link to an actual new blog post when we ship this, but we'll PR it into this branch.
const searchParams = new URLSearchParams({ | ||
code: compressed | ||
}); | ||
window.history.pushState(null, "", "?" + searchParams.toString()); |
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 think it would make sense to replace this with replaceState
, using pushState
makes it very hard to go back from the playground.
Website redesign, stack: