Summary

Fix extension-provided navbar entries in the mobile menu.

Review Request #14154 — Created Sept. 10, 2024 and submitted Oct. 1, 2024, 9:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change adds an optional argument to the templatetag we use to
render the navbar items, which lets us choose a template to use.

Testing Done


Ran unit tests.
Looked and saw that the "Reports" entry added by Power Pack no longer

  looked terrible in the mobile menu.

Commits

Summary	ID
Fix extension-provided navbar entries in the mobile menu. We render navbar entries in two places: the topbar (just under the "Review Board" label) and the mobile menu. In the mobile menu, the navbar entries should be rendered using the sidebar layout/classes, rather than the topbar ones. Unfortunately, extension-provided navbar entries would only render using the topbar layout, which made them look very ugly in the mobile menu. This change adds an optional argument to the templatetag we use to render the navbar items, which lets us choose a template to use. Testing Done: - Ran unit tests. - Looked and saw that the "Reports" entry added by Power Pack no longer looked terrible in the mobile menu.	1643609b199c24510f49d0771b2b24eb1e531584

Summary

Fix extension-provided navbar entries in the mobile menu.

We render navbar entries in two places: the topbar (just under the "Review Board" label) and the mobile menu. In the mobile menu, the navbar entries should be rendered using the sidebar layout/classes, rather than the topbar ones. Unfortunately, extension-provided navbar entries would only render using the topbar layout, which made them look very ugly in the mobile menu. This change adds an optional argument to the templatetag we use to render the navbar items, which lets us choose a template to use. Testing Done: - Ran unit tests. - Looked and saw that the "Reports" entry added by Power Pack no longer looked terrible in the mobile menu.

1643609b199c24510f49d0771b2b24eb1e531584

Issues

Description	From	Last Updated
Can we make this keyword-only?	chipx86	Sept. 18, 2024, 9:04 a.m.
Let's compute this only once outside the loop.	chipx86	Sept. 17, 2024, 4:43 p.m.
nit: How do you feel about settinng the default first, and changing only if sidebar is defined? template_name = "extensions/navbar_entry.html" …	OfficeStapler	Sept. 10, 2024, 10:07 p.m.

flake8 passed.

JSHint passed.

reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 1)

The issue has been resolved. Show all issues

nit: How do you feel about settinng the default first, and changing only if sidebar is defined?

template_name = "extensions/navbar_entry.html"
if sidebar:
    template_name = "..."

david Sept. 10, 2024, 10:06 p.m.

Thanks for chiming in, always happy to have more eyes on our code!

It's a line shorter, but I don't know that it's any cleaner (and with our coding style we'd put a blank line before the if anyway). My general preference in these situations is to have the assignments be at the same level of indentation so it's visually clear that it's one-or-the-other.

Ship it!

```
Ship It!
```

reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we make this keyword-only?
```

reviewboard/extensions/templatetags/rb_extensions.py (Diff revision 1)

The issue has been resolved. Show all issues

Let's compute this only once outside the loop.

Commits:

	Summary	ID
	Fix extension-provided navbar entries in the mobile menu. We render navbar entries in two places: the topbar (just under the "Review Board" label) and the mobile menu. In the mobile menu, the navbar entries should be rendered using the sidebar layout/classes, rather than the topbar ones. Unfortunately, extension-provided navbar entries would only render using the topbar layout, which made them look very ugly in the mobile menu. This change adds an optional argument to the templatetag we use to render the navbar items, which lets us choose a template to use. Testing Done: - Ran unit tests. - Looked and saw that the "Reports" entry added by Power Pack no longer looked terrible in the mobile menu.	d85ef5b294487a42b47aa5efbb146cb47d5ca800
	Fix extension-provided navbar entries in the mobile menu. We render navbar entries in two places: the topbar (just under the "Review Board" label) and the mobile menu. In the mobile menu, the navbar entries should be rendered using the sidebar layout/classes, rather than the topbar ones. Unfortunately, extension-provided navbar entries would only render using the topbar layout, which made them look very ugly in the mobile menu. This change adds an optional argument to the templatetag we use to render the navbar items, which lets us choose a template to use. Testing Done: - Ran unit tests. - Looked and saw that the "Reports" entry added by Power Pack no longer looked terrible in the mobile menu.	1643609b199c24510f49d0771b2b24eb1e531584

Diff:

Revision 2 (+156 -24)

Show changes

	reviewboard/extensions/templatetags/rb_extensions.py
	reviewboard/templates/base/page_sidebar.html
	reviewboard/templates/extensions/navbar_entry_sidebar.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-7.x (aa4a450)