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

Implement main menu #29

Merged
merged 13 commits into from
Aug 8, 2016
Merged

Implement main menu #29

merged 13 commits into from
Aug 8, 2016

Conversation

abitrolly
Copy link
Member

Main menu is a top navigation breadcrumb. It is just a list of pages that are immediate childs of Home page with set flag Promote -> Show in menus.

@staaas
Copy link
Collaborator

staaas commented Aug 4, 2016

@abitrolly let's discuss this feature in person

@abitrolly
Copy link
Member Author

Items are not translated.

<a href="/">Home</a>
</li>
{% for page, is_active in menupages %}
<li>{% if is_active %}<strong>{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would you like to use <strong> instead of class="active"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is no style for "active" nav item (#32) so there is no visual feedback for currently selected menu item.

@abitrolly
Copy link
Member Author

PTAL.

@abitrolly abitrolly added this to the Bootstrap milestone Aug 5, 2016
@staaas
Copy link
Collaborator

staaas commented Aug 7, 2016

I'm working on updating page tree structure these days. I'll update the way we fetch menu items as a part of this.

Another thing I'd like to improve is to have one translated title field.

class='active' doesn't seem to have any visible styles. But this problem also exists in the existing version of filmfest.by.

I'll create issues for the 2 items above.

@abitrolly could you please squash this on merging?

@staaas
Copy link
Collaborator

staaas commented Aug 7, 2016

LGTM

@staaas
Copy link
Collaborator

staaas commented Aug 7, 2016

The issues mentioned above: #33, #34

@abitrolly abitrolly merged commit 0f0150e into kinaklub:master Aug 8, 2016
@abitrolly
Copy link
Member Author

@nott squashed and merged.

@abitrolly
Copy link
Member Author

LGTM is still pending approval. What's going on with that bot?

@abitrolly abitrolly deleted the mainmenu branch August 9, 2016 10:41
@staaas
Copy link
Collaborator

staaas commented Aug 10, 2016

i've switched the bot off several days ago.

abitrolly added a commit that referenced this pull request Aug 25, 2016
Extract top navigation menu structure from wagtail Page tree.
Extract link for root page from HomePage object.
Detect current page in main menu and set style as `active`.
Render only children that have "Show in menu" flag set.
Show translated menu items by using `caption` if available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants