Skip to content
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

Deprecate/remove AMP fit-text setting #4557

Closed
westonruter opened this issue Apr 9, 2020 · 8 comments · Fixed by #5729
Closed

Deprecate/remove AMP fit-text setting #4557

westonruter opened this issue Apr 9, 2020 · 8 comments · Fixed by #5729
Assignees
Labels
Editor Groomed Integration: WP Core P1 Medium priority WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Apr 9, 2020

Feature description

The amp-fit-text setting does not seem relevant after the years since it has been available. I don't recall it being used once by a user. It seems much more suitable for programmatically adding text to a post. For example, it would make great sense to use for auto-generated web stories. But in this context where users are manually entering text, why wouldn't they just set the font size to the desired height on their own? The amp-fit-text ability just seems overly complicated and unnecessary. It should be removed. It also does not appear to even work properly per #5658.

A paragraph block currently has an “AMP Settings” section among its block settings. It should be eliminated, with the “automatically fit text to container” being moved to the Text settings panel. Users should not need to know about “AMP” when they author content.

Also, the amp-fit-text should be assured to work when the content is viewed on a non-AMP page.

image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added this to the v1.6 milestone Apr 9, 2020
@kmyram kmyram added the P0 High priority label Apr 14, 2020
@kmyram kmyram modified the milestones: v1.6, Sprint 28 Apr 14, 2020
@westonruter westonruter added P1 Medium priority and removed P0 High priority labels Apr 16, 2020
@kmyram kmyram modified the milestones: Sprint 28, v1.6 May 27, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 15, 2020
@westonruter westonruter added this to the v1.7 milestone Jul 5, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
@kmyram kmyram added the Groomed label Sep 15, 2020
@jwold
Copy link
Collaborator

jwold commented Sep 15, 2020

We discussed this and plan to deprecate it from the plugin. The exception is that any users who have already implemented this on a page will still see it.

@pierlon pierlon self-assigned this Nov 4, 2020
@pierlon
Copy link
Contributor

pierlon commented Nov 6, 2020

Also, the amp-fit-text should be assured to work when the content is viewed on a non-AMP page.

@westonruter given that the consensus is to deprecate the amp-fit-text control, would you still consider moving the control to the "Text Settings" section and ensuring the amp-fit-text settings work when the content is viewed on a non-AMP page?

@westonruter
Copy link
Member Author

would you still consider moving the control to the "Text Settings" section and ensuring the amp-fit-text settings work when the content is viewed on a non-AMP page?

If the amp-fit-text settings can be moved inside Text Settings then that would be good. I think it wasn't possible at the time so that's why it wasn't done.

Nevertheless, I have doubts as to whether this is being used by many people and it seems more suitable for programmatically adding text to a post. For example, it would make great sense to use for auto-generated web stories. But in this context where users are manually entering text, why wouldn't they just set the font size to the desired height on their own? The amp-fit-text ability just seems overly complicated and unnecessary, no?

@pierlon
Copy link
Contributor

pierlon commented Nov 13, 2020

The amp-fit-text ability just seems overly complicated and unnecessary, no?

Yea that's what I was wondering as well. If it were moved inside Text Settings though, how would the user know that the control is for AMP only? I suppose we could simply prefix the setting title with "AMP" or something along that line.

Also, I think we should also have a set release for when we intend to remove these deprecated settings, so that we can give the users a heads-up on when they will be removed from the plugin.

@westonruter
Copy link
Member Author

Can't the setting be treated the same way as the deprecated blocks? Only show the setting if the block has fit-text setting attributes set. And then show a deprecated notice. In that way, leave it in it's own separate AMP Settings panel as there's no need to move it to Text settings.

You're right that there's no way indicate that the fit-text settings are only applicable to AMP in that case, which is another reason why we should get rid of it if the user is not already using it. This also goes for #4554.

For all of these deprecations/removals, how about we also make sure that we have a link to the support forum for people to voice their concerns about the removal. We may want to put a temporary/deprecated filter in place to force the deprecated stuff to be restored for such users who we find out really need this stuff.

@pierlon
Copy link
Contributor

pierlon commented Nov 13, 2020

Can't the setting be treated the same way as the deprecated blocks?

I don't recall Gutenberg providing such a mechanism, but showing the setting based on whether the block has the attribute set is relatively simple to do.

For all of these deprecations/removals, how about we also make sure that we have a link to the support forum for people to voice their concerns about the removal.

That sounds like a good idea 👍.

@westonruter
Copy link
Member Author

Turns out the feature is broken now anyway per #5658.

@westonruter westonruter changed the title Move AMP fit-text setting to Text settings panel Deprecate/remove AMP fit-text setting Dec 7, 2020
@westonruter
Copy link
Member Author

QA Passed

I added a paragraph block and populate the AMP fit-text setting:

<!-- wp:paragraph {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="5" max-font-size="100" height="100"><p>Fit text with 5≤size≤100!</p></amp-fit-text>
<!-- /wp:paragraph -->

Upon upgrading to 2.1, I see a notice in the console:

image

And the content is:

<!-- wp:paragraph -->
<p>Fit text with 5≤size≤100!</p>
<!-- /wp:paragraph -->

So this shows that fit-text is successfully removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor Groomed Integration: WP Core P1 Medium priority WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants