-
Notifications
You must be signed in to change notification settings - Fork 1
docs: Next.js WP Theme rendered blocks #115
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
base: main
Are you sure you want to change the base?
Conversation
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.
Looks like a good start. Do folks need to include https://developer.wordpress.org/block-editor/reference-guides/packages/packages-base-styles/ ??
examples/next/render-blocks-pages-router/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
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.
This should be its own example, not a modification of a previous example. Thanks.
5b32846
to
0760b8e
Compare
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.
looking good, I'm not seeing a wp-env setup though.
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.
This looks good but there isn't any details on how to setup with wp-env in your README. 🚀
@colinmurphy Sure, readme instructions added. Thank you! |
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.
LGTM 🚀 🚀 🚀
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.
did a quick review of the JS. didn't do any actual testing but in general looks good. I left some quick notes on things I'd do different. That said none are deal breakers.
const fetch = require('node-fetch'); // make sure node-fetch is installed | ||
|
||
async function fetchWpGlobalStyles(endpoint, outputPath = 'styles/hwp-global-styles.css', types = ['variables', 'presets', 'styles', 'base-layout-styles']) { | ||
const typeEnums = types.map(type => type.toUpperCase().replace(/-/g, '_')); |
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.
does this really need RegEx or can it just do standard string replaces?
examples/next/wp-theme-rendered-blocks/example-app/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
examples/next/wp-theme-rendered-blocks/example-app/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
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.
Nice script and example @whoami-pwd 🚀🚀
- Build failed for me, so I've suggested the fixes related to it.
- Suggested changes to the readme.
- Added a script to run the style fetching and included it to the wp-env setup flow.
examples/next/wp-theme-rendered-blocks/example-app/package.json
Outdated
Show resolved
Hide resolved
examples/next/wp-theme-rendered-blocks/example-app/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
examples/next/wp-theme-rendered-blocks/example-app/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
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.
@whoami-pwd This looks good, I can help migrate this to from commonJS to ESM before merging in.
"https://github.com/wp-graphql/wp-graphql/releases/latest/download/wp-graphql.zip", | ||
"https://github.com/wp-graphql/wpgraphql-ide/releases/latest/download/wpgraphql-ide.zip", |
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.
Using the latest versions is good; we might want to consider pinning to a specific version of each plugin to mitigate examples breaking on future updates.
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.
@josephfusco That's a good point.
I created this task here as we also need to consider a few other things - #148
examples/next/wp-theme-rendered-blocks/example-app/scripts/fetchWpGlobalStyles.js
Outdated
Show resolved
Hide resolved
c07baf2
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.
Looks good to me 🚀 🚀 🚀
One small thing (not a blocker) is once the PR is merged, lets create a follow on PR to add the example into the example README.
"https://github.com/wp-graphql/wp-graphql/releases/latest/download/wp-graphql.zip", | ||
"https://github.com/wp-graphql/wpgraphql-ide/releases/latest/download/wpgraphql-ide.zip", |
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.
@josephfusco That's a good point.
I created this task here as we also need to consider a few other things - #148
Also tested locally and looks great ❤️ |
Issue
Provides an example of fetching WP Global Stylesheet: