Update breadcrumb #6514
Update breadcrumb #6514
Conversation
|
I think there is still more to do. But I want to get some feedback first. |
Evaluate the label for the active breadcrumb in a helper to reduce
repetitive view code.
Advantages
* No more overhead by using Rails path helpers
* One place where we set the label for the active breadcrumb
* More generic code, which means we only have to explicitly set the
breadcrumb label for some edge cases.
This replace the usage of path helpers in our breadcrumbs with checks for action and / or controller name. This means that we don't have to worry anymore whether a @project or @Package variable, that was needed by one of the helpers, was set. In addition we stop having to looking up the rooting table only to check if we are on a show or index page.
bgeuken
force-pushed
the
update_breadcrumb
branch
from
64e3d1d to
15c36ae
Compare
|
I agree that the duplication should be avoided, but I also like that everything about the breadcrumbs is in partials. I find that this approach adds another layer (in the helper) and it's not really making it simpler. |
|
simple code isn't really a design goal, de-duplication always wins IMO. |
I agree. |
| @@ -1,6 +1,6 @@ | |||
| = render partial: "webui/#{@container.model_name.singular}/breadcrumb_items" | |||
|
|
|||
| - if current_page?(index_attribs_path) | |||
There was a problem hiding this comment.
What is wrong with this approach? I don't see a benefit in using action_name == ... over current_page...
There was a problem hiding this comment.
We just want to know where we are. There is no need to generate a path. Especially if this can fail, e.g. when the object needed by the generated route does not exist. We had issues like this and alike a couple of times.
The only disadvantage of using action_name / controller_name that I see is that we have to adjust them manually whenever the routes change. But that doesn't happen often.
|
I prefer to follow the KISS principle. |
|
I don't see what is simple in having breadcrumb partials all over the place. And having so many breadcrumb related patches / PRs doesn't really speak for it's simplicity. That said, I agree that this approach isn't perfect either. IMO we should have a simple way to generate them. Without having to define individual partials everywhere. |
Au contraire, mon ami! |
|
Perhaps my statement was too simplified to be generic - which was my point to begin with, so I think I created a recursion. If 'simple code' was a design goal, we wouldn't spend all this effort with models and functions and whatnot. maintable code is the design goal - and I very much agree with @bgeuken here, that the current breadcrumb partials aren't KISS. They are horrible copy&paste code. But I prefer #6532 (comment) over fighting this |
|
Closing for now, since this requires some more polishing |
Evaluate the label for the active breadcrumb in a helper to reduce
repetitive view code.
Advantages
breadcrumb label for some edge cases.