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

Try making skatepark standalone block theme #5523

Merged
merged 14 commits into from
Feb 16, 2022
Merged

Conversation

MaggieCabrera
Copy link
Contributor

@MaggieCabrera MaggieCabrera commented Feb 11, 2022

Changes proposed in this Pull Request:

This PR converts Skatepark from a Blockbase child to a standalone block theme.

  • Some of Blockbase's ponyfills are ported over such as Search block and button styles.
  • The alignments styles that we are using for Archeo, Livro, etc.
  • The header and footer are brought back from Blockbase to Skatepark.
  • We now need a script to load the fonts too
  • The 404 page was being inherited from Blockbase, it is now included on Skatepark
  • Some of the theme.json options from Blockbase needed to be brought to Skatepark as well

Related issue(s):

Closes #5471

@MaggieCabrera MaggieCabrera self-assigned this Feb 14, 2022
@MaggieCabrera MaggieCabrera added the [Theme] Skatepark Automatically generated label for Skatepark. label Feb 14, 2022
@MaggieCabrera MaggieCabrera requested review from kjellr and a team February 14, 2022 15:13
@MaggieCabrera MaggieCabrera marked this pull request as ready for review February 14, 2022 15:13
@MaggieCabrera MaggieCabrera changed the title WIP: Try making skatepark standalone block theme Try making skatepark standalone block theme Feb 14, 2022
@scruffian
Copy link
Member

scruffian commented Feb 14, 2022

I noticed that in the screenshot the social links are on a different line:
screenshot

Now this is a block theme we could go for that layout...

@scruffian
Copy link
Member

Some of Blockbase's ponyfills are ported over such as Search block and button styles.
The alignments styles that we are using for Archeo, Livro, etc.
The header and footer are brought back from Blockbase to Skatepark.
We now need a script to load the fonts too
The 404 page was being inherited from Blockbase, it is now included on Skatepark
Some of the theme.json options from Blockbase needed to be brought to Skatepark as well

I think this is all fine, its the same approach we use for other block themes. . We will probably want to remove this extra CSS when that stuff is in Gutenberg, but we're not adding additional complexity. For the patterns I think we'll want to use the patterns from the directory instead when we can.

Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@MaggieCabrera
Copy link
Contributor Author

I noticed that in the screenshot the social links are on a different line:
Now this is a block theme we could go for that layout...

I tried that but I don't think it's possible unless the social links are decoupled from the navigation block. Either everything is in the same line or everything is vertical, I didn't find a way to make the line break between the links and the social links without adding CSS.

@MaggieCabrera
Copy link
Contributor Author

@kjellr if you give a 👍 we can merge and deploy to .org

@kjellr
Copy link
Contributor

kjellr commented Feb 15, 2022

Thanks @MaggieCabrera! Just a handful of notes on my end:


I'm seeing a lack of left/right padding on small screens:

Screen Shot 2022-02-15 at 2 52 39 PM

Screen Shot 2022-02-15 at 2 52 49 PM


Also, it seems weird that "Next Post" shows up here, even when there's no next post:

Screen Shot 2022-02-15 at 2 55 33 PM

I think the best solution is just to hide those Previous/Next Post titles completely for now. We can probably also remove the block pattern for this as well.


On the homepage, the previous & next buttons appear too close to the posts themselves, so they're easy to miss. Let's add some margin/padding/separator or something to space them out. The extra space should match the space between the "Latest Posts" header and the post below it.

Current Intended
Screen Shot 2022-02-15 at 2 56 38 PM Screen Shot 2022-02-15 at 2 57 17 PM

For some reason, separators in the editor appear less opaque than they do on the front end. Additionally, if you apply a custom color, they lose the custom width we define:

separator


I think we should remove the "Two Columns of Text" pattern. I'm not a fan of the text hierarchy, and it also doesn't scale well.

@MaggieCabrera
Copy link
Contributor Author

Thanks for the review @kjellr!

I'm seeing a lack of left/right padding on small screens:

Yeah, we were missing the correct custom variables for that, it should work fine now

I fixed all the other issues as well, so this should be ready for another review

@kjellr
Copy link
Contributor

kjellr commented Feb 16, 2022

Thanks, @MaggieCabrera! I pushed a couple quick updates:

  • I added the hover rules to the nav items too, in 2e44548.
  • As per @scruffian's note here, I was able to (easily!) implement the intended design for the nav in b4aace2.

nav

This should be ready to go!

@mikachan mikachan merged commit 4d3c803 into trunk Feb 16, 2022
@mikachan mikachan deleted the skatepark-standalone branch February 16, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Skatepark Automatically generated label for Skatepark.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skatepark: Final issues
4 participants