-
Notifications
You must be signed in to change notification settings - Fork 4.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
Pullquote Block #607
Pullquote Block #607
Conversation
@jasmussen The initial commit for pullquote, take a look at the styles and feel free to tweak away, probably will want to move background/font to variables, but easier to play with here without generating conflicts until we're set. The full width implementation is a stand-in until the img full-width solution is set and then can use, likewise for Placeholder. |
Really nice work, Marcus. You've got this. In fact, you may have got this too much :) and I think we might want to pull back a little. I also pulled back a little in some revised mockups here: #547 (comment) That is — some of your ideas here, like color, are really powerful ideas. But we may want to get a V1 of this block merged in without that, and then add it back when we have a good plan for it. The thing is, it's not a live preview of what you'll see in the theme. So how would a theme know what color to apply? If we were to apply the color as an inline color, then we'd want some way for the theme to be able to provide the palette, for consistency. A noble goal, in other words, but probably best tackled separately. We might even want to wait on including the background image for the same reason. I'm going to jump in and try and get the pullquote looking like the latest version of the mockup — big and bold, but with a generic font and generic colors. And a theme can then style this quote via the CSS class it is born with. |
Pushed a few style tweaks. You'll hate me and think I'm terribly boring. But the truth is, you were too good here. You added features that we aren't even ready to add :D
Still need to do:
Can be done later on:
|
Thanks @jasmussen I knew we would want to get colors from the theme, but wasn't sure how. We can tackle that one later - I can do the rest of the clean up for you and get this PR ready to ship. |
Looks like this is ready for final review. @iseulde how's the editable placeholder text looking? It would be nice to use actual placeholder text rather than output it directly like this PR does. |
@jasmussen Yeah, working on it |
Should we merge this in and open a separate placeholder ticket, or should we wait? |
Hm, aren't there tickets? #475 |
blocks/library/pullquote/index.js
Outdated
return ( | ||
<blockquote className="blocks-pullquote"> | ||
<Editable | ||
value={ value || 'Write quote...' } |
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.
- Should be translated via
wp.i18n.__
- Should use ellipsis character for semantic meaning over three dots (
…
)
editor/components/toolbar/index.js
Outdated
@@ -28,7 +28,7 @@ function Toolbar( { controls } ) { | |||
} } | |||
className={ classNames( 'editor-toolbar__control', { | |||
'is-active': control.isActive | |||
} ) } /> | |||
}, control.classNames ) } /> |
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're not using 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.
The change that used the classname got backed out, it was created to pass a class to the control icon and provide a different color. It could be useful to leave for others to be able to tweak controls.
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.
It could be useful to leave for others to be able to tweak controls.
We can always reintroduce it if it proves to be useful, but I'm generally against introducing functionality we expect could be useful but not actually use (YAGNI).
If and when it can be shown to provide value, I think there could also be a discussion on the naming and shape of classNames
. Is it a className
(singular) string? A classNames
(plural) array of strings?
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.
The flip side of this argument might be that we can't always anticipate the needs of a plugin implementing a block. To this point, I think we'll have pretty good exposure in the course of implementing a core set of blocks to the sorts of needs we'll need to accommodate. Or at least, we should consider whether this is an extension point we want to offer. Class names in particular are pretty susceptible to conflict (plugins trying to use the same name), and we may end up doing a disservice by exposing it. There could be alternative solutions such as inline styles, or more built-in customization such as icon color in your original usage.
To be able to color icon controls, needed to support passing in additional classnames to the IconButton.
Revert to a more minimalist design for now.
Sorry that I neglected to revisit this prior to merging other changes this morning, but there are a few revisions that've been made to the quote block that I think are relevant here. Specifically those made in 76d62e7#diff-cda27cec5557c5b032f2b19c44309a1d
You may need to rebase for these changes to take effect. Otherwise, this looks good 👍 |
See also #684 which includes a fix for testing the citation attribute length. |
{ ( citation || !! focus ) && ( | ||
<footer> | ||
<Editable | ||
tagName="footer" |
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.
With tagName
you no longer need the <footer>
wrapper. Currently this will render as <footer><footer></footer></footer>
.
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.
Whoops, fixed in #687
Creates the pullquote block which is a highly stylized quote for dramatic effect, see Issue #547