Add term-description block #29613
Merged
Add term-description block #29613
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
aristath
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
|
Hey Ari 👋 - similar PR opened a few hours ago by @carolinan :#29609. Same comment applies here I think: #29609 (review) and would like your thoughts as well :) |
|
Size Change: +327 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
7 tasks
carolinan
requested changes
Mar 9, 2021
There was a problem hiding this comment.
The two pull requests are very similar but since it looks like we are leaning towards this version, please:
- Add text size, line height, text align and color options
- Generate and include the fixtures: https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/fixtures/blocks/README.md
Use icon from #27989 (comment)
Done, done and done. 👍 |
ntsekouras
reviewed
Mar 10, 2021
There was a problem hiding this comment.
Thanks for working on this Ari! I've left a couple of comments that should be addressed and we can retest again afterwards.
--cc @jasmussen for some design input about the icon and placeholder.
I'd be super happy to help with icon work and tweaks and happy to push directly. I have to work on something else for the remainder of the day, though. Got any screenshots? |
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
|
Thank you for the feedback @ntsekouras! All notes were addressed. 🎉 @jasmussen I used the icon from #27989 (comment) Screenshot with left/center/right aligned blocks, and the icon in use: The placeholder is similar to the one used for content, but some styles (flex) were removed to make alignments work, and also I removed the |
ntsekouras
reviewed
Mar 10, 2021
|
Ah cool, seems like an okay icon for now. About the placeholder text, this is my feeling for the "This is a placeholder for post content" also, we need to do better and be more accurate to the end result. I think there's a longer term opportunity to explore some beautiful prose to pre-fill empty template blocks with, but near term we can still be smarter. It's essentially this field, correct?
Given how rare these are filled out, I will admit it's hard to find something inspirational here. How about: "Description for the current term view." Could also just be "Term description." Anything to avoid "This is the placeholder for", basically 😅 |
ntsekouras
reviewed
Mar 10, 2021
|
The text size option in the placeholder works better now. I agree that the extra class name can be removed. |
Changed to |
Thank you for checking!
Done. |
ntsekouras
approved these changes
Mar 11, 2021
This was referenced Mar 12, 2021
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 adds the
term-descriptionblock.Related to #22724
How has this been tested?
Tested in an FSE theme by adding the block in an
index.htmltemplate.Checklist: