Polish the Site Logo block. #30526
Merged
Polish the Site Logo block. #30526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 tasks
|
Works for me. I wonder if the description can be simplified a little, taking cues from the Site Title block?
Perhaps for devices or scenarios where dragging doesn't work so well? |
Doi, of course. I'll add it back and see what I can do. Probably if it's unset by default, it will be more accurate to the default size. |
|
Hey Joen. It looks like I am not able to build a special Gutenberg plugin by going to the Checks tab -> looking in the left sidebar for Gutenberg Build. It would be nice to see a screenshot (or to test it). I checked the code. Sidebar description says this: That feels like a good and clear description. |
|
Ack, sorry about that. I had taken before and after screenshots, and somehow I forgot to paste them in here! Here's before:
Here's after:
Note that being centered is a bug in my theme, not a bug with the block. Here's the drag and drop zone:
I'll work to restore the width in the inspector now. |
|
Addressed the feedback:
|
|
Size Change: +1.06 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
In terms of the changes proposed in the OP this works well. There's something about that placeholder state though... In the future I wonder if it wouldn't be better to programatically generate a "logo" and insert that. Consequently the thing a user sees after inserting the Site Logo block feels a bit more logo-ish. Does that make sense? Replacing it is just a case of using the already-familiar image tools.
|
|
I don't disagree. But in the name of moving us onward from this:
... do you think maybe this PR could serve as a starting point? |
jameskoster
approved these changes
Apr 7, 2021
|
I do love the idea that we can auto-generate a logo based on site properties. Want to ticket it, or at least suggest it in a comment on #30406? Thanks for the review, I'm going to land this one. |
|
Does it have an upload media link? |
|
Yes, inside the media library. |
|
Of course..:) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR takes a stab at a few of the items from #30406. Specifically it:
I could use advice on the description in the sidebar, is it precise enough?
Note that for 2, I entirely removed the "Logo width" range control in the sidebar. It seemed duplicate compared to the control that exists in the canvas itself, and it was problematic insofar as it showed the image dimensions even when you hadn't explicitly set the width of the image at all (and was therefore subject to theme styles). Is there any reason to keep that control that I'm missing?
How has this been tested?
Test inserting a site logo, test setting it, and that it looks correct in editor and frontend.
Then try resizing the site logo and check that also looks good.
Checklist:
*.native.jsfiles for terms that need renaming or removal).