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

Download page redesign #594

Merged
merged 32 commits into from Oct 7, 2020
Merged

Download page redesign #594

merged 32 commits into from Oct 7, 2020

Conversation

@garbas
Copy link
Contributor

@garbas garbas commented Sep 29, 2020

Design

TODO

  • design headers for both sections (@garbas)
  • design collapsible that scales down to mobile (@garbas)
  • design dropdown to show older versions of Nix/NixOS from the section headers (@samueldr), we can also design this after this PR, but we need to open a ticket so we don't forget.
  • text above navigation is not designed properly, should be above the navigation (@garbas)
  • is More options even needed (@garbas)
  • go over each navigation content pane and implement design (@garbas)

This change is Reviewable

@garbas garbas added the design label Sep 29, 2020
garbas added 2 commits Sep 30, 2020
without the dropdown to the old sections
garbas added 3 commits Oct 4, 2020
disable temporary javascript part
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 5, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/marketing-team-meeting-minutes-14/9304/1

…script
garbas added 2 commits Oct 5, 2020
@garbas garbas changed the title WIP: Download page redesign Download page redesign Oct 5, 2020
@garbas garbas requested a review from samueldr Oct 5, 2020
@garbas garbas mentioned this pull request Oct 5, 2020
7 of 8 tasks complete
Copy link
Member

@samueldr samueldr left a comment

(notes of things to do)

site-styles/mixins.less Outdated Show resolved Hide resolved
<span cls="shell-prompt">$ </span>curl -o install-nix-[%latestNixVersion%] https://releases.nixos.org/nix/nix-[%latestNixVersion%]/install
<span cls="shell-prompt">$ </span>curl -o install-nix-[%latestNixVersion%].asc https://releases.nixos.org/nix/nix-[%latestNixVersion%]/install.asc
<span cls="shell-prompt">$ </span>gpg2 --recv-keys B541D55301270E0BCF15CA5D8170B4726D7198DE
<span cls="shell-prompt">$ </span>gpg2 --verify ./install-nix-[%latestNixVersion%].asc
<span cls="shell-prompt">$ </span>sh ./install-nix-[%latestNixVersion%]</pre>
Comment on lines +37 to +41

This comment has been minimized.

@samueldr

samueldr Oct 6, 2020
Member

s/cls/class/ ??

This comment has been minimized.

@garbas

garbas Oct 6, 2020
Author Contributor

We actually need to change that so that the "$ " comes from shell-prompt class via content:

This comment has been minimized.

@samueldr

samueldr Oct 7, 2020
Member

Why? This would break a rule I follow when writing: content should be in the file, and not in the CSS. This is part of the content here.

If it's about being able to select the content, we have the #no-select(); LESS mixin which could be used here. We might want to apply #no-select(); to something like pre .shell-prompt { #no-select(); }

Another reason not to do it via content is <span class="shell-prompt">[nix-shell] $ </span>. We might even want <span class="shell-prompt">nix-repl></span> 1 + 1!

Co-authored-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
@davidak
Copy link
Member

@davidak davidak commented Oct 6, 2020

I just took a superficial look, since it is not ready.

The 1. 2. 3. implies you have to do all these steps to sucees. That makes no sense.

Screenshot from 2020-10-06 22-51-55

We have discussed the white background before.

You may want to verify the integrity of the installation script

so the menu item should be called "verify installation script"

You can uninstall Nix simply by running rm -rf /nix.

manual says:

The install script will modify the first writable file from amongst .bash_profile, .bash_login and .profile to source ~/.nix-profile/etc/profile.d/nix.sh.

does that also have to be removed to have a clean system again?

maybe that should be better documented in the manual und is not a topic for the download page

the $ in the command in selectable

please notice that light orange on dark orange has a bad contrast and is not very readable!

Screenshot from 2020-10-06 23-04-16

samueldr added 16 commits Oct 6, 2020
LESS compiles out // comments, but keeps /**/ comments.

The /**/ comments are mainly used, here, to keep license attributions.
By prefixing `&` we can make styles in a namespace apply according to
external selectors.

Here `html.with-js &` makes is so that when the selector is expanded,
`&` is replaced with the parent selector (`.collapse`), so it ends up
being `html.with-js .collapse`.

This does have one caveat. The previous implementation would keep the
"un-enabled" variant for any collapse not already converted if the code
ended up stopping due to a crash. This is not something that we should
concern ourselves with, as if it crashes there's more to worry about.
Ideally we'll prefer implementing a generalized behaviour instead of
page-specific JS. But when it's needed, put it at the end so the file is
easier to go through.

Maybe we should invest some time into figuring out a palatable "packer"
for the JavaScript bits? Probably to be looked at later when we choose
the new tooling for the website.
This is because changing the wording of a section will keep deep links
working.

Additionally, I find the generated anchors quite unwieldy.
Using `$("<a />")` is perfectly fine. Even `$("<a class='something' />").
But the moment we want to include variable content, prefer using `.attr`
rather than hoping the content will not destroy our string's HTML.

Sure, we control the DOM, and *it shouldn't happen*. But as we're making
this generic enough, we can make it right at the same time.

Furthermore, this, in my opinion, removes the cognitive load of reading
the escaped and concatenated string. (This is why the double quotes were
replaced with single quotes).
This, coupled with the next change,  reduces confusion when mentally
mapping the model of the collapse.

We don't *really* care about titles, we care about the articles.

This is basically a useless diversion at this point in time, but when
we need to refer to the article in the future, we're already using the
article instead of targeting deeply for its title.
They are the headers, and not outright links. They will be wrapped in
links, though.

(See also the previous commit.)
Since `.wrap()` continues the pipeline returning the original selected
set, this didn't work as expected.
With RSCSS, "modifier" class names are prefixed with a dash.
I had to re-think about all of it before it made sense before the
comments. Not that it's badly done, but this sure will help the next one
who'll read this.
This is helping a bunch with understanding what is going on. Otherwise
`-selected` becomes quite overloaded between the "collapsible" behaviour
and the "selection" behaviour of the tabs-like navigation.
This fixes issues with collapse and passing *through* viewport widths.

The reason is that the activation logic, on load, depended entirely on
which view the site was viewed as!

With this, we instead rely on the behaviour that activating *all* links
is a safe activity.
This helps with synchronization issues by *completely ignoring* actually
synchronizing the elements.

So the behaviour now synchronizes its view *and* the other view in
different manners, depending on what is desired UX-wise.

Read the amended comments in the less file, as it describes the new
behaviour.
@samueldr
Copy link
Member

@samueldr samueldr commented Oct 7, 2020

I added an additional commit where we deviate from the palette by making the orange darker. That commit can be dropped from the change set if desired, though since this style is used in body text, especially in the manual, I figure following the recommendations would be better.

@garbas
Copy link
Contributor Author

@garbas garbas commented Oct 7, 2020

Looks good.

Regarding the colors, I created the issue to double check that our color ratio follows accessibility, #609.

@garbas garbas merged commit 36563bb into master Oct 7, 2020
2 checks passed
2 checks passed
build-and-deploy build-and-deploy
Details
Netlify Netlify deployment
Details
@garbas garbas deleted the redesign-download branch Oct 7, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 5, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-4/9862/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.