loaf icon indicating copy to clipboard operation
loaf copied to clipboard

Add :http_verbs option

Open hoppergee opened this issue 3 years ago • 4 comments

Describe the change

Add :http_verbs option to breadcrumb parameters

Why are we doing this?

When I use this gem on some legacy projects, I met a circumstance which I have to show breadcrumbs on POST/PATCH action. I tried to add an option :http_only at #49. But @piotrmurach suggest me to use http_verbs.

Benefits

Now the current breadcrumb can match with multiple HTTP methods

breadcrumb 'Step 1', onboard_step_path(step: 1), http_verbs: [:get]
breadcrumb 'Step 2', onboard_step_path(step: 2), http_verbs: [:get, :post, :delete]
breadcrumb 'Step 3', onboard_step_path(step: 3), http_verbs: :all

hoppergee avatar Jul 19 '21 14:07 hoppergee

Thank you Hopper for making these changes! Things are looking really good. There is a small style issue left.

There is one dilemma that I'm 'debating' which is the name of the parameter :http_verbs. The verbs term is kind of inaccurate because these are not all verbs and also it is rather 'colloquial' usage. The HTTP rfc talks about HTTP methods. It feels more appropriate to use methods. I'm equally debating whether it should be request_methods or http_methods? What're your thoughts on this? I want to make sure we pick the best configuration name.

Maybe the request_methods is better, as Rack is using request_method.

https://github.com/rack/rack/blob/d15dd728440710cfc35ed155d66a98dc2c07ae42/lib/rack/request.rb#L174-L202

hoppergee avatar Jul 29 '21 10:07 hoppergee

Apologies for not having replied to your request for review earlier. I took a break from open-source. I left a few comments. Let's move this PR forward.

Glad to hear that! I'll get it done sometime this week.

hoppergee avatar Feb 22 '22 04:02 hoppergee

I just fixed all those style issues. Also fixed a wrong proc config test issue

hoppergee avatar Feb 25 '22 10:02 hoppergee

Are there plans to merge these changes?

ankalus avatar May 04 '23 09:05 ankalus