-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[Readd] Stretch text and strech heading variation #73056
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: +210 B (+0.01%) Total Size: 2.41 MB
ℹ️ View Unchanged
|
| } ) ); | ||
| } | ||
|
|
||
| // Hardcode: Collect stretch variations separately to add at the end |
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.
Changes on this file are a contained temporary solution to make Stretch variations don't appear at the top with paragraph and heading. For 7.0 we should have something in the variations API to allow them to appear in a position of their own right instead of close to block implementing them.
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.
Reviewing this change is a good reminder of how disorganised the inserter logic is. I was surprised that this rearrangement was enough to make it work (at least so far in my testing), considering that different inserters do different things, including reordering by frecency.
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.
@mcsf curious why you think it's so disorganised.. It's contained in a getInserterItems selector. Do you mean that the onInsert and the insertionPoints are in different places? 🤔
Personally I don't like this approach with the hardcode hack and I also think an API is not worth it for this single use case.
Hacks like this can set a bad precedence for handling such cases in the code base.
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.
why you think it's so disorganised
I mean that there are subtle differences depending on where the inserter items will be used. In some scenarios, no frecency sorting happens at all; in others, yes. Then there's grouping according to the kind of item and to its origin (first part or third). And so on.
I don't like this approach […] Hacks like this can set a bad precedence for handling such cases in the code base.
I agree :) But I see it as the difference between shipping and not shipping as a block variation (because to me the default placement next to Paragraph and Heading is a dealbreaker).
an API is not worth it for this single use case
I think we'll find something suitable. I just don't know what that is yet.
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.
Hi @ntsekouras,
I share your concern about implementing this hack. I'm not a fan of it either.
But, it seems better than shipping a confusing user experience. The current default order (with "fit text" blocks at 2nd and 4th) will feel broken.
We have three possible paths:
- Implement the hack: A temporary solution to fix the user-facing order immediately.
- Build a new API: The proper, long-term solution, which would require more time.
- Ship as-is: Accept the confusing default order for this release.
While option 1 is not ideal from a code-purity standpoint, it seems to be the most pragmatic choice to avoid shipping a flawed user experience.
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.
To be even more pragmatic: can we come up with any case where this hacks falls apart?
92f5801 to
4db0c30
Compare
|
If we go this path we will probably need better icons. cc: @jasmussen, @jameskoster would you be able to provide some icons or do you think we could use some o the existing ones? |
|
Flaky tests detected in e28eebc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19238031805
|
| } ) ); | ||
| } | ||
|
|
||
| // Hardcode: Collect stretch variations separately to add at the end |
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.
Reviewing this change is a good reminder of how disorganised the inserter logic is. I was surprised that this rearrangement was enough to make it work (at least so far in my testing), considering that different inserters do different things, including reordering by frecency.
84e9c28 to
1ac978d
Compare
bfd62fa to
ce95095
Compare
|
Hi @jasmussen thank you a lot for providing the icons, they were applied: |
|
I'm here to iterate the icons if feedback suggests so: perhaps being so similar to the Paragraph isn't actually helping, and perhaps we need a single "stretch text" icon that is used for both heading and paragraph, same icon? Not clear, but let me know any feedback you hear. One thought, though: if this is a Paragraph block variation, and we also have a Heading block variation, then the variations should be called "Stretchy Paragraph" and "Stretchy Heading", yes? |
It was changed to "Stretchy Paragraph" 👍 |
t-hamano
left a comment
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 PR!
This PR removes the fit text UI entirely. Furthermore, the font size UI is also hidden when the fitText attribute is set to true. This means that to enable fit text, we have to manually add the attribute through the code editor or create a variation with fit text enabled.
This seems unnatural to me. Couldn't we bring back the toggle for fit text like before? What do you all think?
|
Everyone, this PR should be backported to 6.9, right? Adding the Update: Following this PR, I believe #73148 also needs to be backported to 6.9. |
Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: mcsf <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: t-hamano <[email protected]>
|
There was a conflict while trying to cherry-pick the commit to the wp/6.9 branch. Please resolve the conflict manually and create a PR to the wp/6.9 branch. PRs to wp/6.9 are similar to PRs to trunk, but you should base your PR on the wp/6.9 branch instead of trunk. |
Note: This pull request has been correctly automatically cherry-picked to the |
It was correctly cherry picked at 8c2c469 the conflict was because I made a mistake with the labels (the first seemed like the bot did not execute, but it was just a delay and then the bot tried to cherry pick a second time). |
|
We might have to immediately comment when the action starts to signal that it's in progress. |
That would be nice 😄 |
|
@jasmussen Having just encountered those icons for the first time, I note that they only imply stretching and not shrinking. Something that implies both (example 1 / example 2) might make more sense.
I completely agree, and would say that observation is a reason to consider reverting from the variations approach to the toggle. |
|
We're late in the stage, I'd love in put from @youknowriad @jorgefilipecosta and others what you perceive the next steps to be. |
|
It's always easier to add than to remove, so starting conservative is a good move. I think we should take the time to understand user feedback and act in consequence. |
Follow-up of #73056 Workaround for WordPress 6.9. See comment in the patch.
Follow-up of #73056 Workaround for WordPress 6.9. See comment in the patch.
This reverts commit 09e1eb4.
This reverts commit 09e1eb4.



Alternative to #72948.
This PR is an alternative to X as instead of creating a new block it readds two variations "Stretch Text" and "Stretch Heading". It includes a mechanism of excepninally make this variations not appear right above there main blocks. It is a very small contained hardcoded change, we should find an API to do that in a good way in the future.
Fixes: #72947
Screenshot
Testing Instructions
Verify both variations work well.