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

Add About landing page #171

Merged
merged 14 commits into from
Feb 2, 2023
Merged

Add About landing page #171

merged 14 commits into from
Feb 2, 2023

Conversation

adamwoodnz
Copy link
Contributor

@adamwoodnz adamwoodnz commented Jan 17, 2023

Closes #148

Props @tellyworth for initial content

Note that the design uses Courier Prime for the monospace serif font, but our theme has 'IBM Plex Mono', so that's what I'm using for now. I have notified design.

Screenshots

Desktop Tablet Mobile
localhost_8888_about-2_(Desktop) (1) localhost_8888_about-2_(iPad) (1) localhost_8888_about-2_(Samsung Galaxy S20 Ultra) (3)

How to test the changes in this Pull Request:

  1. Build the styles
  2. Add a page at /about-2
  3. Open the page, test responsive behaviour, links, cross browser layout. The cover in particular should fluidly scale between 600px and 1320px

@adamwoodnz adamwoodnz self-assigned this Jan 17, 2023
@adamwoodnz adamwoodnz added [Component] Theme Templates, patterns, CSS [Component] Content Bugs or issues related to the page content labels Jan 17, 2023
@adamwoodnz adamwoodnz added this to the About milestone Jan 17, 2023
@adamwoodnz adamwoodnz marked this pull request as draft January 17, 2023 01:31
@adamwoodnz adamwoodnz force-pushed the add/148-about-page branch 4 times, most recently from 7bb8314 to 4effcb2 Compare January 18, 2023 23:11
@adamwoodnz adamwoodnz marked this pull request as ready for review January 18, 2023 23:11
@StevenDufresne
Copy link
Contributor

Nice! A couple things:

  • The link arrows are still a problem for me since they are internal links. (Issue)
  • The header is black in the designs. Not sure if that was changed from the figma file I am looking at.
  • Based on the same figma, the alignment of the "the four freedoms" content and the number list below is staggered. In this PR it's not. I like this PRs version better but that is a discrepancy i noticed.
  • The space between the four freedoms content and the first freedom is a bit tight, can we make it larger? Not a strong opinion though...
Designs This PR
Screen Shot 2023-01-19 at 8 54 17 AM Screen Shot 2023-01-19 at 8 42 44 AM

@adamwoodnz
Copy link
Contributor Author

  • The header is black in the designs

Weird, the pattern builder generated the template as black-on-white even though the block in the prod page is white-on-black, nice catch 🙏

@adamwoodnz
Copy link
Contributor Author

  • Based on the same figma, the alignment of the "the four freedoms" content and the number list below is staggered. In this PR it's not. I like this PRs version better but that is a discrepancy i noticed.

Yeah good spotting. I found that at certain sizes the large numbers didn't fit alongside the wider columns so I decided to make them consistent with the other sections above (40/60)

@adamwoodnz
Copy link
Contributor Author

  • The space between the four freedoms content and the first freedom is a bit tight, can we make it larger?

Updated in 8b36d9e

@adamwoodnz adamwoodnz force-pushed the add/148-about-page branch 5 times, most recently from 50fcbcf to e12ba2d Compare January 20, 2023 01:08
@jasmussen
Copy link
Contributor

Looks good, I like Plex over Courier, but I'll let Marko chime in once has a chance.

We need to update the pencil strokes per some feedback:

screenshot-2023-01-18-at-13 47 16

The vectors are in this figma, and can be exported as PNGs or SVGs, let me know what works best.

I'm not entirely sure about the pencil strokes further down the page, we can probably omit them from now and include only those at the top as a starting point. What do you think?

@adamwoodnz
Copy link
Contributor Author

adamwoodnz commented Jan 22, 2023

I'm not entirely sure about the pencil strokes further down the page, we can probably omit them from now and include only those at the top as a starting point. What do you think?

Yeah I'm not 100% sure about the positioning of them (mission swirl seems a little random, and four freedoms touches the text), but I quite like the idea of including them further down.

@adamwoodnz

This comment was marked as outdated.

@jasmussen
Copy link
Contributor

Looks good! Nice.

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Looks good - some questions about the code but nothing huge/blocking.

*/

?>
<!-- wp:wporg/global-header /-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this create duplicate header & footer on the page? The header & footer are added in the page template too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, it doesn't seem to, but I'll remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 75634c2


@media (min-width: 1200px) {
.wporg-about-cover-title {
margin-block-end: -42px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know GB uses logical properties, but I don't think we've used them yet — should we be concerned about browser support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a note about where this number comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we be concerned about browser support?

Good question! Looks ok https://caniuse.com/?search=margin-block-end

Copy link
Contributor Author

@adamwoodnz adamwoodnz Jan 25, 2023

Choose a reason for hiding this comment

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

Could you add a note about where this number comes from?

Done in ea616fc


@media (min-width: 600px) {
.wporg-about-cover-title {
margin-block-start: -30px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use the spacing values for these numbers?

Copy link
Contributor Author

@adamwoodnz adamwoodnz Jan 25, 2023

Choose a reason for hiding this comment

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

Done in ea616fc

@adamwoodnz adamwoodnz requested a review from ryelle January 25, 2023 01:02
@adamwoodnz
Copy link
Contributor Author

I think it's more intuitive to use the font family dropdown

I think this is key. I guess I was trying to restrict the usage to certain blocks but that's less important.

keep all the fonts together in the wporg-mu-plugin folder as a convention

If we are treating that as our font library rather than global-fonts as the name suggests, then I think you're right, if there is no load penalty.

@@ -131,19 +131,19 @@
<!-- wp:column -->
<div class="wp-block-column"><!-- wp:list {"className":"is-style-links-list"} -->
<ul class="is-style-links-list"><!-- wp:list-item -->
<li><a href="https://learn.wordpress.org/course/getting-started-with-wordpress-get-setup/"><?php _e( 'WordPress courses ↗', 'wporg' ); ?></a></li>
<li><?php _e( '<a href="https://learn.wordpress.org/course/getting-started-with-wordpress-get-setup/">WordPress courses ↗</a>', 'wporg' ); ?></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these strings should be changing. I think it's a result of the parser changes? In any case, the link tag itself doesn't need to be translated.

I think it would be fine if you drop the changes to these other, non-about patterns, and then iterate on the parser along with the tests PR #181 so that these don't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes this was based on trunk not the parser branch, thanks for that. Now updated and I'll drop the changes to other files in that PR.

@adamwoodnz adamwoodnz changed the base branch from trunk to fix/177-export-content-parse-list-items January 31, 2023 23:07
@adamwoodnz adamwoodnz force-pushed the fix/177-export-content-parse-list-items branch from 22e6f72 to 406e4c2 Compare January 31, 2023 23:15
Base automatically changed from fix/177-export-content-parse-list-items to trunk February 1, 2023 02:39
@adamwoodnz adamwoodnz changed the base branch from trunk to add/184-courier-prime-font February 1, 2023 03:14
@adamwoodnz adamwoodnz requested a review from ryelle February 1, 2023 03:20
ryelle
ryelle previously approved these changes Feb 1, 2023
Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Page and styles look good to me 👍🏻

@adamwoodnz adamwoodnz changed the base branch from add/184-courier-prime-font to trunk February 2, 2023 01:20
@adamwoodnz adamwoodnz dismissed ryelle’s stale review February 2, 2023 01:20

The base branch was changed.

@adamwoodnz
Copy link
Contributor Author

Added Courier Prime and brushstrokes @marko-srb

I'm going to merge and deploy the draft pages this afternoon and we can iterate more during final review phase.

@adamwoodnz adamwoodnz merged commit 0b465e7 into trunk Feb 2, 2023
@adamwoodnz adamwoodnz deleted the add/148-about-page branch February 2, 2023 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Content Bugs or issues related to the page content [Component] Theme Templates, patterns, CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add About landing page
5 participants