Skip to content
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

New landing page #37

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

New landing page #37

wants to merge 6 commits into from

Conversation

leostera
Copy link
Contributor

Not much to see here yet, just started drafting this page.

Moved us to MLX so its easier to see the final output of the page, and started using the dev preview to build this website. Works great!

The workflow is pretty annoying, since I have to run a while true; do dune exec sandworm -- ...; done and a separate server, and then manually reload the browser on every change.

This would be super smooth with something like astro.build but I want to honor the code we've written in OCaml so far – HOWEVER, there's a few things that'd be a lot better if we reused from JS world, like live-reloaders for styles.

I'm also using tailwind css.

This is based off the designs from Claire, with some content/structure adjustments based off Go's and Bun's websites.

Ref: https://www.figma.com/design/vSRUcQqOVTmRZqF1tD2Ov5/Dune-Download-Website?node-id=11-3&node-type=frame&t=LFqNbmPvTj45pZVv-0

image

@leostera
Copy link
Contributor Author

Also yes I am quite jetlagged.

@leostera leostera force-pushed the new-landing-page branch 2 times, most recently from b5df3c1 to 5ce70cc Compare September 19, 2024 13:28
@leostera
Copy link
Contributor Author

Some more changes here:

  • assume all releases have commit + certificate, we don't really care about older ones
  • add a install script based off bun.sh
  • prioritize a quick start but still show how to manually install + verify
  • make room for a FAQ section

@leostera
Copy link
Contributor Author

Updated screenshot:

Screenshot 2024-09-19 at 15 33 24

@leostera leostera marked this pull request as ready for review September 19, 2024 13:59
; has_certificate : (bool[@default false])
; commit : (string option[@default None])
; has_certificate : bool [@default false]
; commit : string [@default "-"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maiste would love to remove this default, just need to clean up the .json file so we drop all the old releases that don't have this info.

Copy link
Contributor Author

@leostera leostera Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for has_certificate – we can drop all old releases without certificates

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, if we restart the website, we can definitely just replace the content of metadata.json by []. This way, we don't have to worry about what we should keep or not. We can consider this as a "fresh start" 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this file before we merge.

Copy link
Member

@maiste maiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a first batch of review, I'll focus on the HTML and testing tomorrow 👍
I still believe we shouldn't push the dune.lock as it is not considered to be portable (in the current situation).

;;
'Linux x86_64' | *)
target=x86_64-unknown-linux-musl
;;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we display an error instead and quit with it is not supported? Otherwise, people might install something that will not work on their system...

Comment on lines +58 to +59
install_dir=$HOME/.dune
bin_dir=$install_dir/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not install it to ~/local/bin as many programs do? It would let you not having to touch the path :)

install Show resolved Hide resolved
case $(basename "$SHELL") in
fish)
# Install completions, but we don't care if it fails
IS_dune_AUTO_UPDATE=true SHELL=fish $exe completions &>/dev/null || :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be mistaken, but I don't thunk we have completion, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, remnants from the original bun.sh install script :D will remove.


echo

case $(basename "$SHELL") in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of directly exporting, what do you think about asking the user? It would prevent people that tries to install the newer version to have the export multiple times and also change the location for the people who want. WDYT?

Comment on lines +56 to +57
let create ~date ~commit targets =
{ date; targets; commit; has_certificate = true}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you turn the metadata.json into a clean version, I think you can simply remove the has_certificate 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you regenerate this opam file, on my machine it says it's different? :D

yarn.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this file? I don't think we need it in our context, no?

Comment on lines -27 to -28
(ocaml-lsp-server :with-dev-setup)
(ocamlformat :with-dev-setup)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot I removed this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't it be in the general .gitignore instead?

@maiste
Copy link
Member

maiste commented Sep 20, 2024

For the header, what do you think about adding more margin to the left and to the right, as in ocaml.org? It's definitely an aesthetic question, though.
image

Copy link
Member

@maiste maiste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here is the second round about the web page 😸

sandworm/lib/web.mlx Outdated Show resolved Hide resolved
Comment on lines +69 to +76
<pre>
"
$ dune init proj hello_world
$ cd hello_world
$ dune pkg lock
$ dune exec hello_world
"
</pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of readability, I would add a background to this. Uniformity is good, and you already put a gray background to the curl cmd.

let faq () =
<section id="faq" class_="flex flex-col py-10 gap-2">
<h2 class_="text-3xl font-black pb-4"> "Frequently Asked Questions" </h2>
</section>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One answer we can put it is the fact that they are unstable. It's most to protect us against any unsatisfied user 😄

Comment on lines +98 to +99
<a class_="text-[var(--accent)]">"Download"</a>
<a>"Cert"</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a choice, or is it a forgetting to not have put the link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just forgot :) will add now.

Comment on lines +115 to +116
let (y,m,d) = bundle.date in
Format.sprintf "nightly-%d-%d-%d" y m d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could be transferred to module Bundle. The smallest operation we do in the web file, the more readable it is (it was an issue in the previous design) 👍


<h2 class_="text-2xl font-medium pt-4 pb-2"> "Installing the binary" </h2>
<p>
"After downloading a binary release of Dune, make it executable and place it somewhere reachable by your PATH:"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to use a way to emphasize the PATH variable, monospace font?

<p>
<span>"To ensure trust in the binary distribution, we generate a build certificate associated with the Github Actions pipeline where the binaries are built. Once you download this certificate, you can use the "</span>
<code>"gh"</code>
<span>"tool to verify it with the following command: "</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<span>"tool to verify it with the following command: "</span>
<span>" tool to verify it with the following command: "</span>

Comment on lines +201 to +203
<span>"To ensure trust in the binary distribution, we generate a build certificate associated with the Github Actions pipeline where the binaries are built. Once you download this certificate, you can use the "</span>
<code>"gh"</code>
<span>"tool to verify it with the following command: "</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe without span is here enough here. WDYT?

<html>
<head>
<meta charset="utf-8" />
<title> "Dune Developer Preview" </title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about extracting the title in a variable? It would make it easier to change in the future (as at some point it will just be Dune Distribution). But maybe it's a problem for later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to reintroduce the plausible script somewhere in this file. Otherwise, we would lose this metric 😢

Co-authored-by: Etienne Marais <dev@maiste.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants