Move new menu item to sub menu#334
Conversation
stklcode
force-pushed
the
develop-submenu
branch
from
68113dd to
07a7d58
Compare
|
I like it. Though with same thoughs on the actual implementation. Hidden menu pages are a pretty nice trick, but I think I'd prefer the "view" parameter here.
Yes, but it's not that many links. Maybe bundle the generation to a helper method like
Really? We don't touch the DOM IDs within our views, so I guess there's no change required on this side.
Sorry for taking over your branch, I've added a |
|
The more the merrier! The view approach would have been more or less the other way I contemplated, so no hard feelings at all. |
| __( 'Statify', 'statify' ), | ||
| 'Statify', |
There was a problem hiding this comment.
Why do we translate the page title but hard-code the menu title? 🤔
There was a problem hiding this comment.
whoops, that one slipped through.
Upon closer inspection we do the same thing in class-statify-dashboard.php 😅
| <ul class="subsubsub"> | ||
| <?php foreach ( $items as $key => $item ) : ?> | ||
| <li> | ||
| <a href="<?php echo esc_url( $item['url'] ); ?>"<?php echo empty( $item['current'] ) ? '' : ' class="current" aria-current="page"'; ?>><?php echo esc_html( $item['label'] ); ?></a><?php echo ( $key === $last_key ) ? '' : ' |'; ?> |
There was a problem hiding this comment.
I would remove the separator from the markup and make it a design property, i.e. move it to CSS like this
| <a href="<?php echo esc_url( $item['url'] ); ?>"<?php echo empty( $item['current'] ) ? '' : ' class="current" aria-current="page"'; ?>><?php echo esc_html( $item['label'] ); ?></a><?php echo ( $key === $last_key ) ? '' : ' |'; ?> | |
| <a href="<?php echo esc_url( $item['url'] ); ?>"<?php echo empty( $item['current'] ) ? '' : ' class="current" aria-current="page"'; ?>><?php echo esc_html( $item['label'] ); ?></a> |
.statify-subnav li:not(:last-child)::after {
content: "|";
margin-left: 0.25em;
}
|
2 participants
I went for the path of least resistance and I'm not sure, this is my preferred solution.
The alternative route would be one merged page. And registering a single slug and switch views via a
?view=parameter insideshow_view(). That would have been tidier, but it touches every internalpage=...link in the three views and the#statify_dashboardselectors indashboard.js.I wanted to keep the edits light instead of doing a big restructuring of the codebase, but if we decide to go for the more complete approach, I'm game.