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

SNOW-25: Change hover colours for menus and put in breadcrumbs #2

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
1 participant
@ghost
Copy link

commented Jul 11, 2018

@jhung
Copy link
Member

left a comment

@grrrero please review the comments.

// Parent page loop
if ( !isset( $parents ) ) $parents = null;
foreach ( $anc as $ancestor ) {

This comment has been minimized.

Copy link
@jhung

jhung Jul 12, 2018

Member

It seems we do a similar action a couple times: take an array and output a list. Can we refactor this foreach loop so it doesn't need to be repeated?

$post_type_archive = get_post_type_archive_link($post_type);
echo '<li class="item-cat item-custom-post-type-' . $post_type . '"><a class="bread-cat bread-custom-post-type-' . $post_type . '" href="' . $post_type_archive . '" title="' . $post_type_object->labels->name . '">' . $post_type_object->labels->name . '</a></li>';
echo '<li class="separator"> ' . $separator . ' </li>';

This comment has been minimized.

Copy link
@jhung

jhung Jul 12, 2018

Member

The separator should not appear as a separate list item as it complicates list navigation for ATs. Instead there are two alternative ways to do this:

  1. Include the separator in the list item containing the text and wrap it in a <span role=presentation> so it is ignored by ATs but visually rendered.
  2. Use CSS and change the list bullet to what you want.

This fix should also be applied in other instances where this is used.

$post_type_archive = get_post_type_archive_link($post_type);
echo '<li class="item-cat item-custom-post-type-' . $post_type . '"><a class="bread-cat bread-custom-post-type-' . $post_type . '" href="' . $post_type_archive . '" title="' . $post_type_object->labels->name . '">' . $post_type_object->labels->name . '</a></li>';
echo '<li class="separator"> ' . $separator . ' </li>';

This comment has been minimized.

Copy link
@jhung

jhung Jul 12, 2018

Member

The separator should not appear as a separate list item. Replace with aforementioned fix.

$cat_display = '';
foreach($cat_parents as $parents) {
$cat_display .= '<li class="item-cat">'.$parents.'</li>';
$cat_display .= '<li class="separator"> ' . $separator . ' </li>';

This comment has been minimized.

Copy link
@jhung

jhung Jul 12, 2018

Member

The separator should not appear as a separate list item. Replace with aforementioned fix.

}
// Check if the post is in a category
if(!empty($last_category)) {

This comment has been minimized.

Copy link
@jhung

jhung Jul 12, 2018

Member

It looks like this if block can be merged into the previous if(!empty($category)) block.

In this particular case$last_category and $category are functionally the same - $last_category will be non-empty if $category is non-empty.

@jhung

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

Closing this PR and issuing a new pull request to address the above issues (as well as other issues).

@jhung jhung closed this Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.