-
Notifications
You must be signed in to change notification settings - Fork 70
171: Better Templates, Pages Examples and Structure #235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with it. If you're good with my changes, go ahead and merge. I'll approve so that you can when you're ready.
components/_data/listitems.json
Outdated
@@ -1,878 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this file in a few components for default content (forms, tabs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. I looked through the "All" page, and didn't see anything. Oh, well. I had contemplated leaving it in for the "example-ness" of them anyway. So I'll revert that commit.
@@ -13,6 +13,7 @@ | |||
</a> | |||
<nav id="main-nav" class="main-nav"> | |||
{% include "@molecules/menus/_menu.twig" with { | |||
menu_class: 'main-menu' | |||
menu_class: "main-menu", | |||
items: menu_items, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires a change in templates/navigation/menu--main.html.twig
to:
{% include "@molecules/menus/main-menu/main-menu.twig" with {
menu_items: items,
} %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
@@ -0,0 +1,12 @@ | |||
{% embed "@templates/_default.twig" with { | |||
menu_items: header.menu_items, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this for the example pages, but I think it's confusing for the template/wireframe ones to link to the pages. I'd rather see their menu items be: Full Width | With Sidebar and link to the correct templates. You?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me too. I had also thought about that. But decided not to just because it would take more time. But if you think it'll be confusing, I'll add another menu set for templates.
See my comments. This is shaping up awesome. Great call on the placeholder for templates. I'd be surprised if we didn't quickly adopt that for all projects moving forward. |
Looks great! If you're happy with my small change, 🚢 it! |
Good catch! Meant to do that, but was trying to work too fast... |
Fixes #171
Changes: