-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Full Site Editing: Add a Site Logo block #18811
Conversation
c61a729
to
b56cfb7
Compare
Some quick feedback:
|
lib/template-loader.php
Outdated
@@ -195,6 +195,7 @@ function gutenberg_find_template( $template_file ) { | |||
|
|||
$_wp_current_template_content = $current_template_post->post_content; | |||
} | |||
$_wp_current_template_content = $current_template_post->post_content; |
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 had to add this line to get templates working. Not sure what I'm doing wrong.
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 had to add this line to get templates working. Not sure what I'm doing wrong.
See also #18877. You may need to rebase.
import icon from './icon'; | ||
|
||
const onError = ( message ) => { | ||
console.log( message ); |
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.
Is there a way to hook into notices without using compose?
6344170
to
f81fb17
Compare
db432db
to
61988b9
Compare
Another round of testing. Here's what I found: Block logo
Uploading
Resizing:
Alignment
Duplicate
Edit as HTML
Reusable Block
Hopefully that is helpful. Sorry for the big ole list! 😬 |
92bb185
to
d508843
Compare
2213bd1
to
7d6df86
Compare
@paaljoachim I think I've addressed all the concerns above, so this is ready for another review :) |
A more unique icon and more integrated with the latest library and styles might help with consistency and identification. |
stroke="black" | ||
strokeWidth="1.5" | ||
/> | ||
<line |
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.
We don't have Line
as a primitive. Maybe we should add it?
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.
Left some comments, this block is very interesting, it highlights the fact that we lack good abstractions to manipulate images without reusing the entire image block. Not something to solve in this PR but I would love some thoughts/follow-ups on this.
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 didn't test a lot but it looks good to me.
Props @pablinos who wrote most of the code.
Description
This adds a Site Logo block to the Full Site Editing experiment.
This uses some filters to non-destructively override the theme mod value when the experiment is enabled and the site logo setting has a value.
How has this been tested?
Screenshots
Known issues
If a non-admin triese to use this block they won't see anything.
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: