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

Improve README.md #14

Closed
wants to merge 1 commit into from
Closed

Improve README.md #14

wants to merge 1 commit into from

Conversation

lex111
Copy link

@lex111 lex111 commented Mar 29, 2017

Any small changes in README:

  • more explanations for the shell commands
  • use default linux prompt (dollar sign)
  • transfer of badges higher

@coveralls
Copy link

coveralls commented Mar 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5738838 on lex111:patch-1 into 721e290 on sjurba:master.

@sjurba
Copy link
Owner

sjurba commented Mar 29, 2017

Thank you for your effort and your interest in my work. I really appreciate that you took the effort to help me improve the documentation.

I am however not sure that I agree with the changes you propose. I will give you my reasons, and I hope I can hear yours if you disagree with me.

One of the most important elements in the README is how to install the app. I want to make that part as clear and easy to use as possible. That is why I have the two commands together in one code block, and why I don't include the linux prompt. This way it is so easy to just copy and paste the whole command in one go. Even when there is just one line to copy, it is still much easier without the prompt as you can just position the pointer or cursor in the margin and select one line down. This is easier than positioning the pointer or cursor between the prompt and the start of the text.

Also although I personally prefer and mostly use yarn, I still feature npm as the default install method, and format the yarn command as a side note. The reasoning is that I want there to be no confusion that the yarn command is an alternative install method over npm, and that most people will still be using npm over yarn. I don't want people to wonder weather they need to run the yarn command as well..

Finally, about moving the badges. For me the badges are not very important, the name of the app and the preamble is more important and should be higher up. And I kind of find it visually nice to have it after the preamble. Especially on mobile.

@lex111
Copy link
Author

lex111 commented Mar 29, 2017

Thank you for the detailed response, I understood you.

Yes, I did not think about Yarn (it's not for nothing that you designed it as a note). Well, the prompt is unlikely to be able to agree (personal preference is more), it looks more understandable, i.e. That it needs to be done in the terminal (although this is usually obvious). But to admit, did not know about GIT_SEQUENCE_EDITOR, so I thought that this is a constant, which need to set in the code 😳.

Badges I would put then under the heading, on a new line, so more familiar. In general, I would think that the first picture should be raised higher, so that it was immediately clear why this is needed, because now this picture is not visible, if you do not scroll the page.

But with all this in mind, I close PR, so there's no more sense in it. Sorry that took your time 😕.

@lex111 lex111 closed this Mar 29, 2017
@sjurba
Copy link
Owner

sjurba commented Mar 29, 2017

No need to apologise. :)

I agree about the image though. I'll move it up.

As for the GIT_SEQUENCE_EDITOR its a bash thing where you can set a environment variable that is only set for the command following the assignment.

Consider this sequence:

$ FOO=foo
$ echo $FOO
foo
$ FOO=bar bash -c 'echo $FOO'
bar
$ echo $FOO
foo

This way you can instruct git to use vim or some other editor only this one time.

@lex111 lex111 deleted the patch-1 branch March 29, 2017 19:40
@lex111
Copy link
Author

lex111 commented Mar 29, 2017

Okay 😉

About the environment variables, I was aware, I mean that at first I was confused because I did not know that there is a variable GIT_SEQUENCE_EDITOR, and if there was an prompt sign, there was no such misunderstanding.

@sjurba
Copy link
Owner

sjurba commented Mar 29, 2017

Right, now I see. This was the initial confusion that made you want the prompt. My bad. Maybe I can clarify a bit in the text then. 😊

@lex111
Copy link
Author

lex111 commented Mar 29, 2017

Thanks, that would be great! 👍

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.

None yet

3 participants