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.

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

Introduce widgets for NavItem #102

Open
joshuadavidthomas opened this issue Jul 16, 2024 · 6 comments
Open

Introduce widgets for NavItem #102

joshuadavidthomas opened this issue Jul 16, 2024 · 6 comments
Labels
🚨 breaking Breaking changes 🏋️ improvement Enhancements or optimizations to existing functionality 🧁 needs baking Requires more time for consideration or development before further action

Comments

@joshuadavidthomas
Copy link
Member

As this library has evolved, it's becoming more and more like django.forms or django_tables2, with a top-level container (Nav, django.forms.Form, django_tables2.Table, etc.) containing child items. Right now the rendering of the navigation is handled completely by the parent Nav, but does it make sense to adopt the widget approach and have a NavItem be also responsible for rendering itself?

@joshuadavidthomas joshuadavidthomas added 🏋️ improvement Enhancements or optimizations to existing functionality 🧁 needs baking Requires more time for consideration or development before further action labels Jul 16, 2024
@joshuadavidthomas
Copy link
Member Author

joshuadavidthomas commented Jul 17, 2024

Of course, the flip side is going this way will require exploring Media and MediaDefiningClass and metaclasses which may be too much of a time commitment and complexity leap for me to make at the moment.

https://docs.djangoproject.com/en/5.0/topics/forms/media/

https://github.com/django/django/blob/252eaca87fc0459dae71c771ca94bf2772127e5a/django/forms/widgets.py#L61-L227

@joshuadavidthomas
Copy link
Member Author

After mulling it over, I almost certainly am going to go down this path, even with the breaking nature of it.

After using django-tables2 for a bit, and past experience with Form, I think this is the right and more Django idiomatic approach.

@joshuadavidthomas
Copy link
Member Author

Hm, of course I'm having second thoughts about introducing this level of complexity. Perhaps it would help to get it out of my head and have a rough sketch of what this approach might look like.

Just going to write a quick rough draft, then let it marinate for a bit.

Current

from django_simple_nav.nav import Nav
from django_simple_nav.nav import NavItem
from django_simple_nav.nav import NavGroup


class ExampleNav(Nav):
    template_name = "path/to/nav_template.html"
    items = [
        NavItem(title="Home", url="/"),
        NavGroup(
            title="About Us",
            items=[
                NavItem(title="Our Company", url="company/"),
                NavItem(title="Jobs", url="jobs/"),
            ],
        ),
    ]

"Field"/Widget Approach (Rough Draft)

from django_simple_nav.nav import Nav
from django_simple_nav.nav import NavItem
from django_simple_nav.nav import NavGroup
from django_simple_nav import widgets


class ExampleNav(Nav):
    home = NavItem(url="/")
    company = NavItem(title="Our Company", url="company/")
    jobs = NavItem()

    about_us = NavGroup(items=[company, jobs], widget=widgets.DropdownWidget)

    class Meta:
        template_name = "path/to/nav_template.html"
        order = [
            "home",
            "about_us",
        ]

@joshuadavidthomas
Copy link
Member Author

So the current approach is obviously much simpler and not much more than a fancy templatetag wrapper class with some helpers around url resolution, flagging the active link, and permissions. All you need to do is write your class and the corresponding template like a normal Django template.

The new approach is much more complex, and I'm not even 100% sure of the API here. The benefit to this approach is that nav items would be aware of how to render themselves, so the parent nav template would be simplified and not much more than a template with a form in it -- instead of {{ form }} it could be {{ nav }}.

And you could have things like dropdowns (seen in the rough draft above), or even avatars, site wide search, notifications, anything you might normally throw in a site nav.

Obviously you can do that with the current approach, but in order to share across multiple projects you need to come up with some way to share the nav template, either copying it over or having a single shared common template. Not the worst way to do things, but I wonder if the widgets would alleviate some of the rough edges of that. Importable widgets in a library, with the ability to write custom one of widgets to customize per project.

@joshuadavidthomas
Copy link
Member Author

joshuadavidthomas commented Jul 30, 2024

Of course, you could introduce widgets without modifying the current Nav structure and API. I've just been working with forms and django-tables2 pretty heavily recently and the rough draft above is how those classes are structured and generally the more idiomatic Django approach. So I wanted to at least write it out and try it on for size to see how it looked.

@joshuadavidthomas
Copy link
Member Author

It seems so obvious now that I've written it out, but I don't need to add widgets to nav items: they are the widgets. So I just need to add widget-like behavior to them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 breaking Breaking changes 🏋️ improvement Enhancements or optimizations to existing functionality 🧁 needs baking Requires more time for consideration or development before further action
Projects
None yet
Development

No branches or pull requests

1 participant