Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

Add footers #54

Merged
merged 40 commits into from
Aug 23, 2024
Merged

Add footers #54

merged 40 commits into from
Aug 23, 2024

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Aug 18, 2024

Description
This PR adds footer patterns.
Closes #38
Testing Instructions

Since the font sizes and font family is not in yet, the main thing to test in this PR is that:

  • The blocks are the same as in the designs.
  • The spacing to the top, bottom, left and right of the footer content, as well as the spacing between the "main" footer and the credits (Designed with...) matches the designs in Figma closely.

Apply the PR.
Confirm that the default footer is used and displays correctly in the editor and front.
Test with different screen widths.
Confirm that the two navigation blocks in the default header has aria-labels. You can do that by viewing the source.

Replace the content in the template part with the other footer patterns from the theme, and test those as well.

(-You will see that the yellow footer is not yellow, the black footer is not black: I added a comment about that)

<div class="wp-block-group alignfull">
<!-- wp:site-title {"level":0,"style":{"typography":{"fontStyle":"normal","fontWeight":"400"}},"fontSize":"small"} /-->
<!-- wp:paragraph {"fontSize":"small"} -->
<p class="has-small-font-size">Designed with <strong>WordPress</strong></p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Translations will be added later when the markup changes are completed.

<!-- wp:navigation-link {"label":"<?php esc_html_e( 'Facebook', 'twentytwentyfive' ); ?>","url":"#"} /-->
<!-- wp:navigation-link {"label":"<?php esc_html_e( 'Instagram', 'twentytwentyfive' ); ?>","url":"#"} /-->
<!-- wp:navigation-link {"label":"<?php esc_html_e( 'X', 'twentytwentyfive' ); ?>","url":"#"} /-->
<!-- /wp:navigation -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not use the social icon block because, well, the design does not use icons.

*/
?>
<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|60","bottom":"var:preset|spacing|50","left":"var:preset|spacing|50","right":"var:preset|spacing|50"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group" style="padding-top:var(--wp--preset--spacing--60);padding-right:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50)">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not changed the background and text color on this footer because I think those colors should be applied by a section style, which has not been created yet.

<div class="wp-block-group" style="padding-top:var(--wp--preset--spacing--60);padding-right:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50)">
<!-- wp:site-title {"level":0,"style":{"typography":{"fontSize":"10vw"}}} /-->

<!-- wp:group {"style":{"spacing":{"padding":{"right":"0","left":"0"}}},"layout":{"type":"constrained","justifyContent":"left"}} -->
Copy link
Contributor Author

@carolinan carolinan Aug 18, 2024

Choose a reason for hiding this comment

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

This whole section should be a separate pattern, which still needs to be created.

@carolinan
Copy link
Contributor Author

carolinan commented Aug 20, 2024

The responsive default footer could use some work,

  • Perhaps it should be displaying as one column with 3 rows.
  • The 100px spacer is definitely not working in my opinion, it is too large.

default-footer-wip

*/

?>
<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|60","bottom":"var:preset|spacing|50","left":"var:preset|spacing|50","right":"var:preset|spacing|50"}}},"layout":{"type":"constrained"}} -->
Copy link
Contributor Author

@carolinan carolinan Aug 20, 2024

Choose a reason for hiding this comment

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

The bottom padding in Figma is 73px, but I was not sure if it was intentional, so I kept it at spacing preset 50.

@carolinan carolinan marked this pull request as ready for review August 20, 2024 10:32
Copy link

github-actions bot commented Aug 20, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: carolinan <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@carolinan carolinan changed the title WIP: Add footers Add footers Aug 20, 2024
@carolinan
Copy link
Contributor Author

carolinan commented Aug 21, 2024

This video is of the default footer.
On smaller screens, the vertical space between the site tagline and the navigation blocks is a bit small:

default-footer.mp4

Centered footer:

centered-footer.mp4

@carolinan
Copy link
Contributor Author

Footer with columns:

footer-with-columns.mp4

@carolinan
Copy link
Contributor Author

Footer with social links:

  • This footer is missing the color change. The background should be black and the text white. This could be added in the pattern, or be implemented as a style variation for the group block.
  • The vertical space between the site title and the navigation is larger than in the design.
social-footer.mp4

@carolinan
Copy link
Contributor Author

Footer with newsletter:

  • I believe the intention is for the site title size to change depending on the browser width, and for this text to always be full width? I was not able to do that. So I have picked 10vw as the font size. This might not be the best choice.
  • The background should be the yellow accent color. This can be added in the pattern, or as a style variation for the group block.
footer-with-newsletter-signup.mp4

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a few comments and suggestions.

<!-- wp:site-logo /-->
<!-- wp:group {"align":"full","layout":{"type":"flex","flexWrap":"wrap","justifyContent":"space-between","verticalAlignment":"top"}} -->
<div class="wp-block-group alignfull">
<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|20","padding":{"top":"0","bottom":"0","left":"0","right":"0"}}},"layout":{"type":"constrained"}} -->
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to limit the width of this group to make it more in line with the designs?

patterns/footer.php Outdated Show resolved Hide resolved
patterns/footer.php Outdated Show resolved Hide resolved
patterns/footer.php Outdated Show resolved Hide resolved
<!-- wp:group {"style":{"spacing":{"blockGap":"var:preset|spacing|20","padding":{"top":"0","bottom":"0","left":"0","right":"0"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group" style="padding-top:0;padding-right:0;padding-bottom:0;padding-left:0">
<!-- wp:site-title {"level":2,"fontSize":"xx-large"} /-->
<!-- wp:site-tagline /-->
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want to limit the width to make it closer to the design.

patterns/footer-columns.php Outdated Show resolved Hide resolved
patterns/footer-columns.php Outdated Show resolved Hide resolved
patterns/footer-newsletter.php Outdated Show resolved Hide resolved
patterns/footer-newsletter.php Outdated Show resolved Hide resolved
patterns/footer-newsletter.php Outdated Show resolved Hide resolved
Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks so much Carolina! 🏅

Looks great, we can iterate on the spacing during a round of DQA.

@carolinan carolinan merged commit 7aa8660 into trunk Aug 23, 2024
3 checks passed
@juanfra juanfra deleted the add/footers branch August 23, 2024 08:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add footers
2 participants