mbr / flask-nav

Easily create navigation for Flask applications.
http://pythonhosted.org/flask-nav/
MIT License
62 stars 37 forks source link

View.active is incorrect when extra arguments are passed to __init__ #17

Closed jacebrowning closed 7 years ago

jacebrowning commented 7 years ago

I have a Flask Blueprint that looks like this:

@blueprint.route("/rooms/<code>")
def detail(code):
    name = request.args.get('name')
    ...

which responds to /rooms/foo?name=bar with:


When I create a View using the same arguments:

view = View(code, 'rooms.detail', code=code, name=name)

It doesn't "activate" when expected:

>>> view.active
False
>>> view.get_url()
"/rooms/foo?name=bar"
>>> request.path"
"/rooms/test
>>> request.url
"http://localhost:5000/rooms/test?name=asdf"

The logic in https://github.com/mbr/flask-nav/blob/8a19d049348424c3b2e7fde97a3128fc4714f516/flask_nav/elements.py#L80 is not prepared to handle this case.

jacebrowning commented 7 years ago

This fixes it for me:

from flask_nav.elements import View as _View

class View(_View):

    @property
    def active(self):
        return request.full_path == self.get_url()

Should I submit a PR for this fix? Does this work in all cases?

mbr commented 7 years ago

I believe this is working as intended, the query string should not influence what navigational item you are seeing. Some common usecases:

/search?q=foo vs /search?q=bar

In this case, both should probably activate the search menu item.

Same with stuff like /page_foo?lang=en vs /page_foo?lang=de.

You can configure it either way, but I prefer the current default to be the general case. However, subclassing is the right way to change this behaviour if you need to.

jacebrowning commented 7 years ago

the query string should not influence what navigational item you are seeing

From my testing, it seems that that query string does influence the active item.

Neither /search?q=foo nor /search?q=bar will activate a /search route. The query string needs to be stripped from View.get_url() in order to match /search.

mbr commented 7 years ago

I stand corrected, you're right. It's not working the way I thought it should work and even then, not as intended. I'll fix this in a bit.

mbr commented 7 years ago

Have a look at f1e3c1a and the laster master branch. It should fix the issue, in your case, you probably want to set ignore_query to False. Let me know if that works for you.

jacebrowning commented 7 years ago

I am unable to reproduce this issue in f1e3c1a using the default options. Thanks!

I'd appreciate a 0.6.dev1 release so that I could pin my requirements.

mbr commented 7 years ago

Released as 0.6.