CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Time to Read: Replace toggles with block variations #71897
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
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +34 B (0%) Total Size: 1.95 MB
βΉοΈ View Unchanged
|
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.
Maybe these are really two different blocks then? |
Another approach might be to add word count as a block variation, similar to how the Post Date block has a Modified Date block as a variation. I don't have a strong opinion on what approach is best π€ |
One block inserted twice, or separate blocks, both afford a little customizability as far as apparance, insofar as you might insert both in a row block and put your own separator between them, or something else. Not a strong opinion, but past experience tells me this flexibility is welcomed by theme designers. |
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
72570d7
to
e07a79b
Compare
I've updated this to use a block variation. |
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.
Thanks for the update.
Not a blocker, but should we update the icon for the Word Count block variation? This is because the "clock" icon is used in both blocks.
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Yeah I think so, but not sure what to use! |
Maybe @jasmussen or @jameskoster can help with an icon. Having different icons will also render the icons for variation switcher in inspector controls. |
displayAsRange, | ||
showTimeToRead, | ||
showWordCount, | ||
displayMode, |
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.
Not from this PR, but as I was checking this file I saw that the return parts.map( ( part, index ) => (
part above is not needed. We always push
one item to parts
array, so we should directly return the value in each if
block that we now do parts.push
@scruffian Finally, can we fix the unit test that is failing? |
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.
LGTM!
Let's merge this PR first and follow up with code quality improvements and icon changes.
Because not including this PR in Gutenberg 21.8 will cause backward compatibility issues. The 21.8 RC release process should start within the next 1-2 days. See #71897 (comment)
What and Why?
Based on this feedback, I changed the way this block works so that its not possible to remove all content from the block.
How?
Replaces the toggles with block variations
Testing Instructions
Screenshots or screencast