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

Added a link to all installers to the homepage #1370

Merged
merged 2 commits into from
May 28, 2018
Merged

Added a link to all installers to the homepage #1370

merged 2 commits into from
May 28, 2018

Conversation

t-botz
Copy link
Contributor

@t-botz t-botz commented Mar 7, 2018

re #1253

I have just resolved conflict of #1270 and adapted to the new index page.

Here is how it looks:
otheros2

@t-botz
Copy link
Contributor Author

t-botz commented Mar 8, 2018

r? @alexcrichton

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

I think this is a good change, thanks for rebasing the PR. I'd like to fixup the links a little bit though, see comment inline.

www/index.html Outdated
@@ -26,22 +26,25 @@
<div id="platform-instructions-unix" class="instructions" style="display: none;">
<p>Run the following in your terminal, then follow the onscreen instructions.</p>
<pre>curl https://sh.rustup.rs -sSf | sh</pre>
<p class="other-platforms-help">You appear to be running Unix. Click <a class="default-platform-button" href="#">here</a> to see the installer for all supported platforms.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Could we rephrase these links to avoid the 'click here' anti-pattern, e.g., make the link text "view all supported installers" or something like that

Copy link
Contributor Author

@t-botz t-botz Mar 8, 2018

Choose a reason for hiding this comment

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

What about : "You appear to be running Unix. If not, [display all supported installers]."

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

@nrc
Copy link
Member

nrc commented Mar 8, 2018

ping @withoutboats any comments on this?

@nrc
Copy link
Member

nrc commented Mar 22, 2018

ping @alexcrichton and @withoutboats - any thoughts? I'm inclined to merge this with my review comment addressed.

@alexcrichton
Copy link
Member

Sounds good to me!

@Diggsey
Copy link
Contributor

Diggsey commented May 27, 2018

Hey @thibaultdelor, any chance you could update the PR with that change? Thanks!

@t-botz
Copy link
Contributor Author

t-botz commented May 27, 2018 via email

@t-botz
Copy link
Contributor Author

t-botz commented May 28, 2018

Here you go. I have also slightly reduced the font so it looks on one line in windows. It was looking weird otherwise.
Before:
image
After:
image

@t-botz
Copy link
Contributor Author

t-botz commented May 28, 2018

r? @Diggsey

@Diggsey
Copy link
Contributor

Diggsey commented May 28, 2018

Thanks, looks great!

@Diggsey Diggsey merged commit a835916 into rust-lang:master May 28, 2018
@t-botz t-botz deleted the linkAllInstallers branch May 28, 2018 12:54
AJ-Ianozi pushed a commit to AJ-Ianozi/getada-download that referenced this pull request Mar 9, 2024
Added a link to all installers to the homepage
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.

5 participants