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.

Sign up for GitHub

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

Jump to bottom

Fix links with directory_indexes #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix links with directory_indexes #17

wants to merge 4 commits into from

Conversation

vinc
Copy link
Contributor

@vinc vinc commented Nov 22, 2017

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
Copy link
Owner

marnen commented Nov 22, 2017

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
Copy link
Contributor Author

vinc commented Nov 22, 2017

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
Copy link
Contributor Author

vinc commented Nov 22, 2017

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

@marnen
Copy link
Owner

marnen commented Nov 22, 2017

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
Copy link
Owner

marnen commented Nov 22, 2017

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
Copy link
Contributor Author

vinc commented Nov 22, 2017

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
marnen reviewed Oct 19, 2018
View reviewed changes
spec/breadcrumbs_spec.rb Outdated Show resolved Hide resolved
marnen
marnen reviewed Oct 19, 2018
View reviewed changes
spec/breadcrumbs_spec.rb Outdated Show resolved Hide resolved
@marnen
Copy link
Owner

marnen commented Oct 19, 2018

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

@vinc
Copy link
Contributor Author

vinc commented Oct 19, 2018

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
Copy link
Owner

marnen commented Oct 19, 2018

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
Copy link
Contributor Author

vinc commented Jul 26, 2020

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
Copy link
Owner

marnen commented Jul 26, 2020

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

@marnen
Copy link
Owner

marnen commented Jul 26, 2020

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

vinc and others added 2 commits July 26, 2020 20:04
@aw-was-here
Copy link

aw-was-here commented Oct 13, 2020 •
edited
Loading

FWIW, there are other people who would like for this to work. 👍

@marnen
Copy link
Owner

marnen commented Oct 13, 2020

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

@aw-was-here
Copy link

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
Copy link
Owner

marnen commented Oct 13, 2020

@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
Copy link
Owner

marnen commented Oct 13, 2020

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

@vinc
Copy link
Contributor Author

vinc commented Oct 13, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marnen marnen marnen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vinc @marnen @aw-was-here