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 some clarifications #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ogrotten
Copy link

@ogrotten ogrotten commented Apr 3, 2018

These are the things that weren't very clear while I was going thru the docs on my first run with pug. Will monitor for changes and can resubmit for the next half-day or so.

Hope it helps.

[edit] also this is my first public PR so please be gentle!

Copy link
Member

@ForbesLindesay ForbesLindesay left a comment

Choose a reason for hiding this comment

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

I've made a couple of inline comments on things I think could be improved further. Overall I agree that this is an improvement, and it's great to get a relative newcomer's eyes on our docs, as it's hard to see what's missing after a while. Add a comment to let me know when you want me to take another look. I'm really keen to get this merged in.

h2.blue Description
p.description.
User has no description,
why not add one...
else
h2.red Description
p.description User has no description
p.description User is not Finn.
Copy link
Member

Choose a reason for hiding this comment

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

Would this be clearer as:

  if user[id].name === "Finn"
    h1.yellow Name
    p.name= user[id].name
  else
    h2.red Description
    p.description User is not Finn.

  if user[id].description
    h2.green Description
    p.description= user[id].description
  else
    h2.blue Description
    p.description.
      User has no description,
      why not add one...

i.e. first, are they "Finn", second, do they have a description, with a space between the two if blocks.

Copy link
Author

Choose a reason for hiding this comment

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

The first section makes sense, I've changed it.

The second section, I was trying to work in the boolean style 'if exists' check on the more sensible authorised element.

@@ -12,12 +12,22 @@ Plain text does still use tag and string [interpolation](interpolation.html), bu

One common pitfall here is managing whitespace in the rendered HTML. We'll talk about that at the end of this page.

## Free text

Simple free text can be added to the page, without a tag, by using a pipe. Note: Free text like this has various caveats regarding styling, etc.
Copy link
Member

Choose a reason for hiding this comment

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

We have been calling this "piped text", so this section should be merged with that section that's further down the page. It may be that:

  1. Free text may be a clearer name than "piped text" it's probably closer to what people are looking for.
  2. It might be better to have the piped text further up the page.
  3. Your description may be clearer than the one we have currently.

I'm happy to hear opinions on this, and I don't have a strong opinion on it of my own. As a relative beginner, you're probably much better placed than I am to suggest what the best wording is for the help files.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah see I had no idea that's what Piped Text was intended for. . . it came across as "piped text is a continuation of text in an element", with the existing example being a continuation of p element. I tend to agree that my example makes the intended point a bit more clearly.

@ForbesLindesay
Copy link
Member

P.S. Congratulations on your first public PR!

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

2 participants