Fix navigation issues #1685
Fix navigation issues #1685
Conversation
|
I've added a The purpose is to enable navigation to the root in a multi page application with no-logo.
|
|
I've applied the same changes to the material and golden. Had to update vanilla a bit to keep things consistent. If won't touch bootstrap now as it does not work for me at all. So I cannot really test these changes in isolation. Looks nice also with logo, site title and app title.
and the original behaviour without a site title still works.
|
|
Ready for review @philippjfr . I would need this to showcase things at awesome-panel.org and for work because I run multipage apps. |
MarcSkovMadsen
added
status: ready
type: enhancement
|
And let me know if there is anything I can do to make reviewing and merging this easier. Thanks. |
| @@ -29,13 +29,11 @@ | |||
| <button class="material-icons mdc-top-app-bar__navigation-icon mdc-icon-button">menu</button> | |||
| {% endif %} | |||
| <div class="title-bar"> | |||
| <span class="mdc-top-app-bar__title"> | |||
There was a problem hiding this comment.
We should continue using this class, can you achieve what you need by adding a second more readable class?
| {% if app_title %} | ||
| <span id="app-title"> | ||
| <a class="title" href="" >{{ app_title }}</a> | ||
| </span> |
There was a problem hiding this comment.
Simply because bootstrap does not Work for me. Cf. Yesterdays test report and comment above.
There was a problem hiding this comment.
This PR is not against the latest version, you seem to forget pulling before you start making PRs 🙂
There was a problem hiding this comment.
I don't believe so. I'm quite sure I git fetch, git pull holoviz master when I start. I think its because you do work in the mean time. Otherwise there is something technically I don't understand.
There was a problem hiding this comment.
Ok. I can see you merged the bootstrap fixes yesterday. I must be doing something wrong with git. I simply don't understand it.
| @@ -38,7 +38,7 @@ def test_template_with_sidebar(template_class=pn.template.VanillaTemplate, theme | |||
| logo=LOGO, | |||
| theme=theme | |||
| ) | |||
|
|
|||
| vanilla.site="My Site" | |||
There was a problem hiding this comment.
Not a big deal but why not set this in the constructor?
There was a problem hiding this comment.
Background: The site is as "optional" parameter for users that serve multiple applications. They can specify it in the constructor the acceptsparams. They can also specify it afterwards. Just as they can for the title.
Is your question why I did not make site a named parameter in the __init__ function?
If your question is why I did not specify it in the constructor, then the answer is that I wanted to test the callback that is set up. Just as for the title.
|
I will start fixing now. Should I UPDATE: I WILL DO. |
|
Ready for review @philippjfr I've fixed your comments (except the site comment) and also fixed the boostrap padding+scrollbar problems from the testreport #1680 |
|
Thanks, looks good! |
For users serving multiple apps/ pages the templates provide no way to navigate to the root of the site. This PR will provide that. See #1684.
So far I've added navigation to the root of the site from the logo for all templates.