gma / nesta

File Based CMS and Static Site Generator
http://nestacms.com
MIT License
902 stars 121 forks source link

Add "link text" metadata and Page#link_text method. #107

Closed MicahChalmer closed 12 years ago

MicahChalmer commented 12 years ago

This implements the "link text" metadata as discussed in detail in the comments of pull request #104. In so doing, also addresses #89 by adding "HeadingNotSet" and "LinkTextNotSet" exceptions.

One thing that might look odd at first glance: I changed the behavior of menus so that, for the home page only, if no heading or link text exists then it will use "Home" as the link rather than raising an exception. The reason I was concerned about this is:

  1. The default home page template (that gets created via "nesta new") has no heading or link text
  2. The first sample menu you see in the documentation includes the home page at the top. So does the menu-editing page in the content demo.
  3. Before this change, having the home page in your menu when no heading is specified results in a menu with an empty link, which does not produce anything visible in the browser. Not what's intended, I'm sure, but it doesn't look bad unless you actually look at the HTML code.

I didn't want simple sites that followed the sample documentation before to suddenly throw exceptions--even coherent ones. The same override was already there for the breadcrumb bar, so I think it makes sense to apply it to the menu as well.

gma commented 12 years ago

I've just been having a good look through. Could we split this commit into two?

  1. One that deals purely with the addition of the new link_text method, and
  2. Another that deals with the menu linking to the home page.

If I've understood your explanation correctly (2) is nothing to do with this link text stuff, and could go in a separate pull request entirely. Is that right? Feel free to git push -f to your link-text-metadata branch. I can see where you're coming from with the concern about throwing exceptions, but I'd rather have smaller steps in the changelog. I'm also not sure that the breadcrumb has that right (maybe throwing exceptions would be better); it'd be good to see it as a separate patch.

I also noticed we've gone a few characters beyond 80 columns in page_spec.rb; fixing that too would be great.

Cheers, Graham

MicahChalmer commented 12 years ago

Done. I actually split it into three--the first just adds the new attribute, but doesn't change everything else to use it; the second makes everything use it, and the third is the menu change. The reason I include it in the same pull request is that I think including the first two commits of this without the third makes it break sites that used to work, for no obvious reason.

I fixed the line of mine in page_spec.rb that was more than 80 characters. I think there is still one more in there, but I didn't want to add an extraneous diff that has nothing to do with what this pull request does. There's some more existing >80 chars stuff in the project too.

The other good thing is that I rebased it to after your change to fix #108, which allowed me to get rid of an ugly thicket of begin/rescue stuff in Page#title that I really didn't like. It was needed to preserve the logic of using the parent's title in a page's default title, but that's moot now that Nesta doesn't do that anymore.

gma commented 12 years ago

Cracking, thanks Micah. Splitting it is appreciated...

gma commented 12 years ago

I've just merged the first two commits of the three (but haven't pushed yet).

I'm not sure about the behaviour of the third (MicahChalmer@76d3afd6decf41453c0d591802033d9b1bffd6ef); it will use the home page's link text in the menu to link to the home page, rather than "Home". In #108 @chadoh points out this isn't what he'd expect, and (having tried it on nestacms.com) I agree with him.

The third commit also modifies the text that's used to link to the home page in the breadcrumb; it'll use the page title (if it's set) in preference to "Home". Setting a site's home page's title tag is a fairly common thing to do, and they'd usually be quite chunky, so not suitable for menus or breadcrumbs.

@MicahChalmer – did you feel strongly about it? I could have overlooked a scenario here; it wouldn't be the first time…

On the issue of breaking sites that used to work; I'll release it as 0.10.0 and plaster a big paragraph about it in the release notes.

MicahChalmer commented 12 years ago

My concern was this scenario:

  1. create a new site with nesta new,
  2. add a menu.txt file with a / line in it to link to the home page, without modifying index.haml

Under nesta 0.9.13 that created an empty link (i.e. it put <a href="http://whatever.example.com/></a> in a menu item), which did not render at all in a browser and might not be noticed. In your current master, after you merged MicahChalmer@a663325, it throws a LinkTextNotSet exception.

I haven't made the breadcrumb (or the menu) use the page title instead of "Home." It would use a heading, if one was added, if that's what you meant. And no, I don't feel strongly about making it always use "Home" on the breadcrumb instead. I do think the menu should behave the same way, rather than outputting empty links or raising exceptions. I don't think it's a big deal either way. Another way to fix it is just to change the default index.haml to have a link text or heading. Or just warn in the documentation that you need to add that if you want to link to the home page from the menu.

gma commented 12 years ago

Thanks for clarifying – good job I checked. I've deprecated the breadcrumb_label helper and renamed it to link_text. It handles the LinkTextNotSet exception as you suggested. I've also added the link text metadata to the default home page.

Once the file-based caching problems that were introduced in 0.9.13 are sorted, it'll go out in 0.10.0. Cheers for persevering with it!