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 in instructions for installing gitk #1495
Conversation
I'm not sure if organizing this into two separate lists is as good as just annotating the existing list:
Would it make sense to just do a single liner, something like this: diff --git a/app/views/downloads/download_linux.html.erb b/app/views/downloads/download_linux.html.erb
index 29f6c991..3f52ccf9 100644
--- a/app/views/downloads/download_linux.html.erb
+++ b/app/views/downloads/download_linux.html.erb
@@ -10,12 +10,17 @@
The latest version is
<%= link_to @latest.name, "https://www.kernel.org/pub/software/scm/git/git-#{@latest.name}.tar.gz"%>.
+ Note that on many platforms, the `gitk` viewer ships as a separate
+ package; the instructions below include it but installing it is
+ optional.
+
<h3>Debian/Ubuntu</h3>
<p>For the latest stable version for your release of Debian/Ubuntu</p>
<code># apt-get install git</code>
<p>For Ubuntu, this PPA provides the latest stable upstream Git version</p>
<code># add-apt-repository ppa:git-core/ppa</code>
<code># apt update; apt install git</code>
+ <code># apt install gitk</code>
<h3>Fedora</h3>
<code># yum install git</code> (up to Fedora 21)<br> (with a similar update for each of the other OS blocks)? |
Sure that is a good idea. Above the code block saying: <code># apt install gitk</code> Should we have a message like the following: <p>Optionally, if you would also like to install gitk, then run the following:</p> To inform the user that the next command will install gitk. |
My thinking was that the block at the top would explain that for all of the individual OS's below, and we wouldn't have to repeat it (and thus we could avoid making the already-long list even longer). But it should only be one extra line per OS to say "To install gitk:", so maybe that's not too bad. |
I don't see any suggestions, reviews, or conflicts currently on this pull request. Is there a reason that the merge is being delayed? I have added all the commits that I need to. |
Sorry @Yash-Singh1 , but sometimes we can't look at PRs immediately. Between daily job, personal life, vacations, and other tasks, some days we don't simply have the time to look at PRs in the same day. Still, we try to be fast to give feedback :) thank you for your contributions so far, helping improving the website and documentation! 🥇 (I made some small comments) |
No problem. I thought that you guys were thinking that I was still putting in commits earlier. |
padding-top: 0; | ||
margin-top: 0; | ||
} | ||
</style> |
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.
Sticking a <style>
tag in the body is a bit unusual. I think @pedrorijo91's comment in #1495 (review) meant to put it into app/assets/stylesheets
(probably in downloads.css
?).
Edit: but see above, where I think maybe going back to your original double-tag solution is the best.
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.
@peff I don't know the layout of the codebase, and there wasn't any downloads.css
inside the app/assets/stylesheets
directory. What should I edit?
.maincontent pre { | ||
height: 10px; | ||
overflow-y: hidden; | ||
padding-top: 0; |
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 don't think this matches what <code>
does (we still have more margin/padding on the non-top sides). It might be easier to reuse the css we use there. Or, looking at the existing css, it really does look like the intent was that one would do <pre><code>...</code></pre>
as you had originally (I say that because there are selectors that would match code
inside pre
).
That involves fewer styling decisions for us to make here, plus it doesn't carry any risk of changing some other unexpected part of the page.
Closes #1489
This pull request refers to #1489: Downloading Git does not automatically download gitk on apt-based systems. Requested by @peff
Added in a new section inside the https://git-scm.com/download/linux page.