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.
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
[AMP Stories] Ability to resize text block. #1972
[AMP Stories] Ability to resize text block. #1972
Changes from 7 commits
c6ab80a
08a8612
aa453a1
9f210db
685d187
5d24258
006bd89
55d5212
38fcca4
f24f1d8
898ab53
7ce2945
0cc8ee3
1919ff0
ffd0271
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@swissspidy I've added the amp-fit-text's replication logic here. Note that I didn't make any changes to
fontSizePicker
(yet), so I renamed the PR back to WIP.Testing this it feels like maybe it would be good if the user could still assign a custom font size, too. Perhaps we could even have just only two values for this: Auto (amp-fit-text logic) and Custom. Thoughts?
Also, let me know if you know of a better place for adding the logic, note that it needs to check the element's changing width and height to calculate the font size so the element should be rendered.
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.
At the moment I see two reasonable options here:
ToggleControl
(like in the post editor) that is enabled by default.The font size picker would only be displayed when this toggle is disabled. Screenshot:
* Worth noting that the font options come from the
withFontSizes
HOC (and thus editor settings), not sure how easy it is to override these.Besides this, I'd probably extract
calculateFontSize
in its own class method or even move it tohelper.js
as it is static.Also, I am not sure if this should use
setFontSize()
or use a new attribute, e.g.autoFontSize
. But that probably makes more sense when using option 2 from above (toggle). That way you can set a custom font size, turn on the toggle, turn it off, and get back your custom font size from before.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.
Good idea, makes it consistent with other options too.
That's also a good point, especially since it would potentially be good to use it in case of other blocks at some point as well and not just with AMP Stories.
Hmm. There are some cases where it might be unexpected to switch back to an old custom font size, however, there are more cases where it makes sense. 👍