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 in instructions for installing gitk #1495

Closed
wants to merge 21 commits into from

Conversation

Yash-Singh1
Copy link
Contributor

@Yash-Singh1 Yash-Singh1 commented Aug 9, 2020

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.

@peff peff temporarily deployed to git-scm-pr-1495 August 10, 2020 08:54 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 10, 2020 19:42 Inactive
@Yash-Singh1
Copy link
Contributor Author

I fixed the header type for Mageia. Also, is there any way to fix the problem where the code blocks are auto-wrapping and going off the screen:
Screenshot from 2020-08-10 12-46-26

@peff
Copy link
Member

peff commented Aug 11, 2020

I'm not sure if organizing this into two separate lists is as good as just annotating the existing list:

  • it means we mention each OS twice, which makes the page much longer

  • we have two really long similar-looking lists, which makes it easy to accidentally scroll from one to the other and not realize you've done so

  • I imagine most people would find their OS of choice and read the instructions for it, never realizing that if they kept looking they'd find more details

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)?

@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 16:36 Inactive
@Yash-Singh1
Copy link
Contributor Author

Yash-Singh1 commented Aug 11, 2020

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.

@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 17:03 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 17:14 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 17:17 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 17:23 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 20:57 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 11, 2020 23:07 Inactive
@peff
Copy link
Member

peff commented Aug 12, 2020

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.

@peff peff temporarily deployed to git-scm-pr-1495 August 12, 2020 20:17 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 12, 2020 20:24 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 12, 2020 20:26 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 12, 2020 20:37 Inactive
@peff peff temporarily deployed to git-scm-pr-1495 August 12, 2020 21:22 Inactive
@Yash-Singh1
Copy link
Contributor Author

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.

@pedrorijo91
Copy link
Member

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)

@Yash-Singh1
Copy link
Contributor Author

No problem. I thought that you guys were thinking that I was still putting in commits earlier.

@peff peff temporarily deployed to git-scm-pr-1495 August 17, 2020 21:49 Inactive
padding-top: 0;
margin-top: 0;
}
</style>
Copy link
Member

@peff peff Aug 17, 2020

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.

Copy link
Contributor Author

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;
Copy link
Member

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.

@peff peff temporarily deployed to git-scm-pr-1495 August 18, 2020 20:46 Inactive
@pedrorijo91 pedrorijo91 temporarily deployed to git-scm-pr-1495 November 4, 2020 23:03 Inactive
@pedrorijo91 pedrorijo91 closed this Oct 9, 2023
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.

Downloading Git does not automatically download gitk on apt based systems
3 participants