search

marnen / middleman-breadcrumbs

Breadcrumbs helper for Middleman (http://www.middlemanapp.com)
MIT License
11 stars 12 forks source link

Fix links with directory_indexes #17

Open vinc opened 6 years ago

vinc commented 6 years ago

Here's a problem I found: when you add activate :directory_indexes to config.rb, the links in the breadcrumb stay the same.

If I have two pages, /sources/foo/index.html.erb and /sources/foo/bar.html.erb and I visit /foo/bar/ in my browser (with pretty URLs) I expect to find links to /foo/ and /foo/bar/ in the breadcrumb. But that not the case, instead it's the usual /foo/index.html and /foo/bar.html.

Here is how to reproduce the problem on a fresh install:

middleman new foo
cd foo
mkdir source/foo
echo "---\ntitle: foo\n---\n" > source/foo/index.html.erb
echo "---\ntitle: bar\n---\n" > source/foo/bar.html.erb
echo "gem 'middleman-breadcrumbs'" >> Gemfile
echo "activate :breadcrumbs" >> config.rb
echo "activate :directory_indexes" >> config.rb
sed "s/<body>/<body><%= breadcrumbs(current_page) %>/g" source/layouts/layout.erb -i
bundle
bundle exec middleman

With this PR we get the expected links.

But there's just one catch that I found so far, without pretty URLs activated you still get /foo/ instead of /foo/index.html. Though that isn't a problem for me.

P.S. I'd like to be able to run the test suite with bundle exec rspec to see if I break something and add a test for my PR but couldn't get it working. Sorry about that.

marnen commented 6 years ago

Thank you for this and the other issues you opened; I'll be happy to look at them over the next few days.

However, one thing that you should know...I won't merge code if it doesn't have tests, so it's in your interest to get the test suite running in your development environment. What happened when you tried?

vinc commented 6 years ago

No worries, I'm with you on not merging without tests! How do you run the test suite though?

rpec isn't in the gemspec, and when I add it, well, all tests fail when I run bundle exec rspec.

vinc commented 6 years ago

I just checked Travis, you run bundle exec rake test, okay!

marnen commented 6 years ago

Yeah, it's Minitest::Spec on this project, not RSpec.

That was an experiment, really; if it gets too unwieldy, feel free to switch it to RSpec.

marnen commented 6 years ago

Also, Guard is all set up for this project, so you can just have it running in the background instead of running tests manually.

vinc commented 6 years ago

Okay I fixed the existing tests by adding a method url to the object page and fixing the method path to return the actual path (without the first "/" like middleman does) instead of a URL.

I'll come back to this PR soon to add some real tests of the problem and its resolution.

marnen commented 6 years ago

I'm not sure I understand what problem this PR is solving. Can you give me a concrete example of the "bad" behavior?

vinc commented 6 years ago

Other than:

If I have two pages, /sources/foo/index.html.erb and /sources/foo/bar.html.erb and I visit /foo/bar/ in my browser (with pretty URLs) I expect to find links to /foo/ and /foo/bar/ in the breadcrumb. But that not the case, instead it's the usual /foo/index.html and /foo/bar.html.

The "bad" behavior is this gem doesn't seem to work with pretty URLs, my fork does.

Could be something wrong with my config, especially now that this PR is one year old.

marnen commented 6 years ago

Sorry, I think I was reading too fast and overlooked some of your problem description. Let me take a less hurried look and see what I can figure out.

vinc commented 4 years ago

Hello, did you have a chance to get a look at this PR to make breadcrumbs and directory indexes work together?

I don't see any other users encountering this case in the issues of this repo since the creation of this PR though, so it doesn't look very important at all and I've been using my fork in the meantime.

marnen commented 4 years ago

Let me refresh my memory on this and see what I can do.

marnen commented 4 years ago

...but in the meantime, would you please respond to my review comments?

aw-was-here commented 4 years ago

FWIW, there are other people who would like for this to work. :+1:

marnen commented 4 years ago

@aw-was-here Fantastic. Can you help by writing tests so this code can get merged?

aw-was-here commented 4 years ago

If I knew more ruby, I'd totally help out. Sorry! [As someone else involved with open source, I get how frustrating that is.]

marnen commented 4 years ago

@aw-was-here Out of curiosity, if you don't know Ruby well, how are you using Middleman at all, and why would this feature be useful to you in the first place?

@vinc Can we get this to a state where I can merge it?

marnen commented 4 years ago

(Travis failures seem to be related to #20, not any code here.)

vinc commented 4 years ago

To be honest after reading again the PR I'm not happy with using URI(page.url).path instead of page.path to get the pretty printed path, it feels like a hack. A better fix should perhaps be in middleman itself? I'll try to have a look at that.