-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Gutenberg: Add Jetpack colophon to Jetpack blocks #27901
Conversation
That's a great PR description, thank you so much for your effort! Generated by 🚫 dangerJS |
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.
Works good, looks good, code is good!
I left a couple of small questions.
|
||
const JetpackBranding = () => ( | ||
<InspectorControls> | ||
<div className="jetpack-branding"> |
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.
Can we set the className
directly on InspectorControls
?
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.
No, we can't - which is why I did it that way.
@@ -0,0 +1,23 @@ | |||
/** @format */ |
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 is specific to the editor, should the file/Component name reflect that?
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.
Well, it's also specific to the sidebar in the editor - how specific should we get in the naming? I think it's good as-is, considering that there isn't a bunch of other components at this point 😉
I believe a lot of design discussion around this has already happened, but the branding just seems way too obtrusive. It feels like an ad to me. The color, the size... it all seems like too much. And I think it sets a bad example for plugin developers. |
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.
Code's good.
I think we have to site on this for the time being sadly. Thanks for putting this together. |
No worries - it was a good experiment, as we saw a use case for reusable components that make sense only in gutenberg context in Calypso. Should we close it for the time being @MichaelArestad? |
Yes. Just read an update around that. Let's close this for now. Thanks Marin! |
Changes proposed in this Pull Request
/components
directory in/gutenberg
that we can use for reusable components that make sense only in Gutenberg context.JetpackBranding
component that renders aJetpackLogo
with an<hr />
above, usingInspectorControls
.Screenshots
Markdown:
Related Posts:
Tiled Gallery:
Testing instructions
Fixes #27773