-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Tag Cloud: Add typography supports (except font size) #43452
Conversation
While testing this with Twenty Twenty-Two and Twenty Twenty-Three, I am getting a JS error when I try to change the font family setting.
|
I can replicate the issue @carolinan reported. The same error is encountered if you try to add font family support to the Archives block.
|
@Mamaduka, can you please share your input on how can I fix this issue? I saw a similar issue was fixed by you in the PR #40468 Just for the trial, I tried to add a condition similar to lock for font family and it give me a different error saying Thank you. |
@amustaque97 thanks for highlighting #40468, it lead me to what I think is the cause of the problem when switching font family. I have a PR up to fix this in #43937. |
@amustaque97 #43937 has been merged so if you can rebase this PR, we should be in a good position for a final review 👍 |
dc8814c
to
f2fb087
Compare
Indeed that fix works 👏🏻 . Here is a small recording of the font family activity for the tag-cloud block. Screen.Recording.2022-09-07.at.8.15.30.PM.mov |
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 working on this one @amustaque97 👍
I've taken it for a spin, and for most of the supports adopted, it works well. Unfortunately, the text-decoration styles are not applied cleanly at the individual block level or via theme.json/global styles.
We might need to tweak our approach to adopting typography supports for this block. An alternative approach might be:
- Skip serialization of the typography supports
- Update the server-side rendering of the block to generate the classnames and styles and apply these to the inner links
- Use
__experimentalSelector
to define a feature level selector for these typography supports so theme.json/global styles can generate applicable styles - Tweak the block styles such that theme.json and global styles apply the text-decoration
The current state of the work adding typography supports to the Search block might provide some clues although the approaches will differ somewhat.
users should not be encouraged to setting font-family locally, which is why it should not be visible by default (ellipsis menu) on block instances
The font family control shouldn't be exposed by default so we'll need to tweak that in the block.json. See #39642 (comment) for a full explanation.
The following suggestions are nitpicky, but they will help others understand the scope of this PR better when they come across it for historical context.
- Change title to "Tag Cloud: Add typography supports (except font size)"
- Update the What? section: "Add typography support to Tag-cloud block (except font size)"
- Replace the copy/paste error in the Why? section: "Allows typographic styling of Tag Cloud links"
- Correct the How? section as this PR doesn't adopt all typography supports and shouldn't show font-family by default
- Add test instructions for theme.json and global styles so more reviewers can help out
Unrelated to the work in this PR, there are a couple of follow-ups we should look at as well:
- When either smallest/largest font size fields are cleared the block crashes
- These font size fields should be relocated to the Typography panel (via SlotFill).
Thanks again for tackling this block @amustaque97, I appreciate the edge cases around some of these block supports can provide a few headaches.
Thanks @amustaque97 for updating the PR title and description. Sorry for the slow re-review on this, I was waiting for the updates to remove the font family as a default control and switching to the new approach to solve the issues noted in my last review. I'd be happy to pick up this PR and implement my suggestions if that helps. Let me know 🙂 |
@aaronrobertshaw, thank you for checking with me 🙂 Sorry I was quite busy with my office work because of the release. I have removed the font family as default control Let me know if there is any work pending in this PR. Regarding follow-up,
can we take it up as a part of a different issue? WDYT? |
Thanks for iterating on this one @amustaque97 👍
There is still the issue with applying text-decoration styles cleanly, as noted in my earlier review.
I took a closer look at what might be involved in skipping the serialization of typography supports and applying them directly to the inner links. Given the tag cloud is generated via We have a few options given this:
As this block already diverges from the standard adoption of all typography supports (see font size), I don't think it would hurt to omit text-decoration support as well. The Tag Cloud block is a dynamic block so there isn't any saved markup in the post content that would force us to write deprecations if we were to change our approach in the future and skip serialization (maybe after those core functions were extended?). I propose we just remove the text decoration support from the Tag Cloud block and proceed with the block supports that do apply cleanly. What do you think? |
this is done because of the serialization issues with the typography support as mentioned in the comment. #43452 (review)
I agree with you @aaronrobertshaw, sorry for not checking the comment properly. I have removed the
Yes, maybe I'm not sure TBH 😓 |
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 updating this one again @amustaque97.
LGTM! 🚢
Note: If we do decide to inject styles into the tag cloud's markup later on, we might be able to leverage the WP_HTML_Tag_Processor
.
Related:
What?
Why?
How?
Testing Instructions
Example theme.json snippet
NOTE: Font size is already present and was merged as a part of #37311
Screenshots or screencast