-
Notifications
You must be signed in to change notification settings - Fork 359
(DON'T MERGE) Only display default credit info if FSE is not active. #1612
Conversation
We want to use the footer credit block in FSE instead.
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 think you have moved the site-info
into the wrong conditional block 😅
It's inside this:
Lines 26 to 38 in dde2d7c
if ( has_nav_menu( 'menu-2' ) ) : ?> | |
<nav class="footer-navigation" aria-label="<?php esc_attr_e( 'Footer Menu', 'varia' ); ?>"> | |
<?php | |
wp_nav_menu( | |
array( | |
'theme_location' => 'menu-2', | |
'menu_class' => 'footer-menu', | |
'depth' => 1, | |
) | |
); | |
?> | |
</nav><!-- .footer-navigation --> | |
<?php endif; |
But instead it should go at the end of this:
Lines 23 to 39 in dde2d7c
else : // Otherwise we'll fallback to the default Varia footer below. | |
get_template_part( 'template-parts/footer/footer', 'widgets' ); | |
if ( has_nav_menu( 'menu-2' ) ) : ?> | |
<nav class="footer-navigation" aria-label="<?php esc_attr_e( 'Footer Menu', 'varia' ); ?>"> | |
<?php | |
wp_nav_menu( | |
array( | |
'theme_location' => 'menu-2', | |
'menu_class' => 'footer-menu', | |
'depth' => 1, | |
) | |
); | |
?> | |
</nav><!-- .footer-navigation --> | |
<?php endif; | |
endif; |
Ah great catch! Does 34c3287 look right? |
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.
Changes in footer.php
LGTM!
Though, there are some SCSS changes going on at the moment with the Varia themes, that might break stuff unexpectedly.
Since this change does not have any styling changes, do you mind removing the CSS files from the PR before merging?
@Copons those are expected style changes (I updated the PR description and left a comment) which simply removes an FSE override for the footer credit area. This shouldn’t break anything since that style only existed if FSE was active.
However you do bring up a good point. Should we just wait to merge until the Dotcom integration of the footer credit block is ready? If we merge this and then deploy varia, current FSE sites won’t see a footer credit area.
…On Nov 1, 2019, 4:25 AM -0700, Jacopo Tomasone ***@***.***>, wrote:
@Copons approved this pull request.
Changes in footer.php LGTM!
Though, there are some SCSS changes going on at the moment with the Varia themes, that might break stuff unexpectedly.
Since this change does not have any styling changes, do you mind removing the CSS files from the PR before merging?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@noahtallen Ah, you're right, there are a few unrelated CSS changes but also related ones, sorry about it. Anyway, yes let's wait for the Site Credit block, and deploy in sync! 👍 |
I'll wait to rebase until closer to release |
this will be superseded by proper FSE themes. |
We'll be using the FSE footer credit block instead.
Note: This should NOT be deployed until the footer credit is included on WordPress.com.
Changes:
Testing:
Related issue(s): Automattic/wp-calypso#37012