[Site Logo]: Add logo replace flow in Inspector controls #49992
Conversation
| .block-library-site-logo__inspector-upload-container, | ||
| .block-library-site-logo__inspector-media-replace-container { | ||
| button.components-button { | ||
| color: $gray-900; |
There was a problem hiding this comment.
Most of these styles were copied from the style variations buttons. Should we consider a new 'variant' of button if we plan on reusing them? 🤔
There was a problem hiding this comment.
Reusing is my only remaining concern about this PR, because exatcly yes, we should have as little custom CSS as possible in PRs like these, and ideally reuse componentry whenever possible.
What would it require in this place? Do the style variation buttons need to be canonized as part of the Button style? Or is it a toggle? And if we canonize those can we reuse them here? If either answer is yes, it would be great to do that, either here or as a followup. Thanks so much for working on this.
There was a problem hiding this comment.
In my last commit I removed some obsolete css, so it's way less now. Aside of that, these outline buttons seem to be a combination of secondary variant Buttons and a few overrides. I think it would make sense to have a specific Button variant for such styles.
Noting though that these styles can be used probably in just a div or something and it would be good to have a common styling - not sure where it should live in code though.. An example is this PR too, where we use similar styles(not the hover ones) for users that have no permissions to update, so we render the info in a div.
I think we can handle this in a follow up, but 🤷 . Whatever you prefer.
|
Holy guacamole, that's impressively fast work! Looks almost ready to go. I'm just about to head out ofr the day, but I expect we can greenlight this quickly next week. Thank you. |
|
Size Change: +2.2 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
| { canUserEdit && !! logoUrl && ( | ||
| <SiteLogoReplaceFlow | ||
| { ...mediaReplaceFlowProps } | ||
| name={ <InspectorLogoPreview /> } |
There was a problem hiding this comment.
It's slightly surprising that this just works; I guess it's more of a "label element" than a name.
The destination component, MediaReplaceFlow, doesn't type its props in the code but it does document them in its README file. It turns out that the name is supposed to be a string. What should we do here? Leave that component untouched and work around? Or amend MediaReplaceFlow to explicitly accept elements? I wonder if it's safer to add a separate prop (like label) or just make name polymorphic. I have no opinion here; maybe the latter option is enough.
There was a problem hiding this comment.
There was a problem hiding this comment.
It's working because internally name is just interpolated in the JSX:
gutenberg/packages/block-editor/src/components/media-replace-flow/index.js
Lines 147 to 155 in c602985
Under a technical point of view, it feels safe to change the docs and allow name to be any ReactNode. I guess it depends more on whether you'd prefer to "limit" consumers of the component to pass strings of text for design / semantics / any other reasons.
Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>
|
Really really nice work, incredibly close right out of the gate. Thank you! This is virtually ready to land. Here's a GIF showing all the inspector cases for the new media button: A few small things:
This should either say "Add media" (I could see us allowing a video site logo in the future) or "Add image" if we want to postpone the more generic term.
The button should be 40px tall (indeed we should be deprecating 36px, see #46734). There should be a prop to leverage this. In the mockup, I have image width grouped with "Media", but I'm waffling on that one. Since it's already in the Settings panel, maybe we keep that and revisit this aspect when we can refresh the Image inspector?
In the mockups, I imagined also seeing the file extension of the file:
To me that's actually about indicating what that field is about, actual file management. It's not critical we get this right, especially if this is a challenge, but I think it would be a nice improvement. The media field that holds the filename and circular thumbnail is meant to be an ItemGroup, because theoretically in the future it could become a layer management interface.
So conceptually it's similar to other ItemGroups, like the recently landed Duotone:
I don't know how many styles we get for free by actually using the ItemGroup here, but hopefully the metrics. The item should be 40px tall:
The thumbnail should be 20x20:
And it should have a small gray overlaid semitransparent border, like color swatches do:
Hopefully using the components should net all those for "free". Nice work. Small fixes, and IMO this is ready to land. |
|
It might be good to update the featured image inspector UI to match this as a follow-up? |
|
Yes, good enhancement. And Cover, honestly. Cover might need a little design. |
|
I think I addressed the feedback. Let me know if anything more is needed.
I agree. We can follow up on that one. |
|
Thank you for such fast work! This is really close, and could honestly land as is. Small note, There are a couple of nitpicky questions that mostly relate to the componentry and how it's used, but it reveals itself primarily in how these hover styles present themselves. For example in the empty state, the hover state of media button is animated and makes the outline thicker, making it appear as if it's focused: For that style of button (gray-bordered), that's not the correct style compared to the style variation buttons which also have the gray border. Related to that, the filled in media button also has this hover style: But ItemGroup items do not: Overall, we should unify on a single hover style that's the same across outline buttons an itemgroups aside, which is mainly about a blue hover style. It can also be a blue outline, like the Style variation buttons, but they should not be thicker like the focus style. So my main question is: where are these hover styles inherited from? Ideally we shouldn't need any custom CSS at all for these components. I'm not personally averse to it, but just noting that our components should be able to handle these things natively, so that if one day we advance the hover style of the buttons, they are advanced across all instances. |
|
For now I updated the styles introduced here to match the |
There was a problem hiding this comment.
Visually and on its behavior, this one is ready:
I shared some general notes on componentry reuse here: #49992 (comment) — I will defer to devs on the componentry on how when and where to unify here. It's a fuzzy rule: generally we should not write custom CSS for pieces like these, as it's all meant to be reused base componentry. But I wouldn't block this PR for it. We just need to all pull in that direction so that if we were to, say, update the border style on the componentry level, all linked components would inherit the new styles, instead of having local overrides that causes drift.
|
This is a great improvement, thanks for working on it! |
|
I added a comment here in the closed issue: #49273 (comment) |
|
@ntsekouras @jasmussen I was just testing this for 15.7 and found that if the file name for the image is really long, the image will get constrained below 20px. We may want to fix ahead of the 15.7 release on Wednesday. cc @bph
|
What?
Resolves: #49273
This PR adds the actions for set, replace and reset the site logo in Inspector controls. For users with no permissions to upload, if a logo exists it will show the 'chip' only.
Testing Instructions
Screenshots or screencast
Screen.Recording.2023-04-21.at.5.46.56.PM.mov