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

Refactor NavGroup and NavItem to add a render method #70

Open
3 tasks
joshuadavidthomas opened this issue Apr 17, 2024 · 0 comments · May be fixed by #73
Open
3 tasks

Refactor NavGroup and NavItem to add a render method #70

joshuadavidthomas opened this issue Apr 17, 2024 · 0 comments · May be fixed by #73
Labels
enhancement New feature or request

Comments

@joshuadavidthomas
Copy link
Member

joshuadavidthomas commented Apr 17, 2024

When adding the type hints to #69 I realized that I don't want the get_items method to return a list of RenderedNavItem and have someone have to deal with the overhead of an additional extra class. Or add additional typing to return a list of RenderedNavItem or NavGroup | NavItem. That's just sloppy and unnecessary.

Instead, the NavGroup and NavItem should have a render method with any associated helper methods to go along with that. It would bring them more in line with the Nav as well and its render method.

I never really liked the solution I came up with regarding the rendering of the items, but the extra RenderedNavItem class seemed the most simple solution at the time. After sitting with it for a while, this way seems cleaner and somehow more simple. Not sure how I didn't see it the first go around.

  • Add render methods to both NavGroup and NavItem
    • Instead of an extra RenderedNavItem class, it could instead be a mixin that both of them inherit to cut down on duplication. Though the number of helper methods responsible for rendering is not that many, so that maybe overkill and just some small duplication would be better. 🤷‍♂️ Table that for now until PR review.
  • Remove RenderedNavItem
  • Change Nav.get_items method to call the render method for each item. Additionally, adjust the type hint to list[NavGroup | NavItem]
@joshuadavidthomas joshuadavidthomas added the enhancement New feature or request label Apr 17, 2024
@joshuadavidthomas joshuadavidthomas linked a pull request Apr 18, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant