inveniosoftware / flask-breadcrumbs

Flask-Breadcrumbs is a Flask extension that adds support for generating site breadcrumb navigation.
https://flask-breadcrumbs.readthedocs.io
Other
30 stars 23 forks source link

More than one variable in route causes problems #41

Open drummerwolli opened 8 years ago

drummerwolli commented 8 years ago

I try to get to work routes which have more than one variable part with the dynamic list constructors (I can't find an example for that unfortunately). The application works itself, but the links are set wrong. in this specific case, the link on code1 are completely wrong set.

My code to demonstrate the problem (should be executable with python3 app.py. I tried to strip it down to the bone to show the problem, most of the stuff is lend from the few examples):

from flask import Flask, render_template_string, request

from flask_breadcrumbs import Breadcrumbs, register_breadcrumb

breadcrumbs_tpl = """
<div>
{%- for breadcrumb in breadcrumbs -%}
    <a href="{{ breadcrumb.url }}">{{ breadcrumb.text }}</a>
    {{ '| ' if not loop.last }}
{%- endfor -%}
</div>
"""

app = Flask(__name__)
Breadcrumbs(app=app)

@app.route('/')
@register_breadcrumb(app, '.', 'Home')
def index():
    return render_template_string(breadcrumbs_tpl + '<br><a href="topic1">go down</a>')

@app.route('/topic1')
@register_breadcrumb(app, '.topic1', 'Topic 1')
def topic1():
    return render_template_string(breadcrumbs_tpl + '<br><a href="topic1/5">go down</a>')

def code1_dlc(*args, **kwargs):
    code1 = request.view_args['code1']
    return [{'text': code1, 'url': code1}]

@app.route('/topic1/<code1>/')
@register_breadcrumb(app, '.topic1.code1', 'sub_topic1', '', dynamic_list_constructor=code1_dlc)
def sub_topic1(code1):
    return render_template_string(breadcrumbs_tpl + '<br><a href="sub_topic2">go down</a>')

@app.route('/topic1/<code1>/sub_topic2')
@register_breadcrumb(app, '.topic1.code1.sub_topic2', 'sub_topic2')
def sub_topic2(code1):
    return render_template_string(breadcrumbs_tpl + '<br><a href="sub_topic2/7">go down</a>')

def code2_dlc(*args, **kwargs):
    code2 = request.view_args['code2']
    return [{'text': code2, 'url': code2}]

@app.route('/topic1/<code1>/sub_topic2/<code2>')
@register_breadcrumb(app, '.topic1.code1.sub_topic2.code2', '', dynamic_list_constructor=code2_dlc)
def sub_sub_topic2(code1, code2):
    return render_template_string(breadcrumbs_tpl)

if __name__ == '__main__':
    app.run(debug=True)

Or am I doing it wrong?

jirikuncar commented 8 years ago

@drummerwolli thanks for reporting. I can't see anything right now, but I will try to have a look during next week if time permits.

drummerwolli commented 8 years ago

btw: this might not be a bug in flaskbreadcrumbs, but in flask-menu, since breadcrumbs is just taking the urls from menu as far as i can see ...

metasoarous commented 7 years ago

Perhaps you intentionally did this just for testing purposes, but 'url' should map to an actual url, not just the code2, 'code1', whatever param. You'd probably want to do something like flask.url_for('sub_sub_topic2', code1=code1, code2=code2). I have something like this almost working but am experiencing some weird issues I'm not able to track down easily. I'll try to post something if I figure anything out, but at the moment things just feel borked and I'm getting ready to drop the lib (we don't 100% need breadcrumbs, but thought they would be nice for our users).

metasoarous commented 7 years ago

I realized that the problem I was having was that if there are any route/query params in the lowest level of the hierarchy (index, in the example above), that page loads fine but nested pages fail to, even if those pages handle their own breadcrumbs/params using dynamic_list_constructor. I realized this is because url_for is implicitly constructed via when you are loaded that top level page, but as soon as you try to load a nested page, it needs to construct that url, but can't because it doesn't implicitly know what parameters need to be passed. So you end up having to use the dynamic_list_constructor all the way down in this case.

Now that I've thought through thiis, the reasoning makes sense, but it's far from trivial to see that you would have to do this from the get go. And again, the errors one gets are very confusing because the breadcrumb works when you load that top level route. I'm not sure that there's any better way around this as far as the API goes (maybe there is, maybe there isn't), but at the very least, it would be nice if there was more helpful documentation on this sort of issue.