-
Notifications
You must be signed in to change notification settings - Fork 385
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
Conversation
…y/1965-text_block_resize
@swissspidy / @westonruter At this moment I ended up adding only the right and the bottom resizers -- otherwise the resizer should change the positionTop and positionBottom percentages as well to not make it move weirdly: if resizing from the left edge would just change the width while the positionLeft would remain the same, then actually the right edge would move for the user instead of the left one. Looks like the Image block has handled for right align only (the left resizer only shows up if the image is aligned right and the right one is hidden in this case). We could follow the same pattern in case we'd want to add align to the Text block. Thoughts? Let me know if you think it would be critical to add the resizers to all edges or if left and bottom would be OK for now. |
I guess in this case positionLeft and the width would need to be changed.
I think the current status works for now. Three things I noticed in a quick test: Odd initial size The initial width is too small, making the Write something! placeholder span over two lines: Block Mover and Resize Handles always visible As a user I don't want to see these controls while typing. This works quite well in the post editor: all controls disappear while typing, and the block mover only appears when hovering near the left side of the element. Text overflows container It would be awesome if we could use However, the code for I think we can easily adapt this script (mainly the In the post editor the plugin already adds a "Automatically fit text to container" control to enable this on the front end, but we might want to make this smarter for Stories. Perhaps we can always use amp-fit-text, no matter what, or add a new "Auto" option to the "Font Size" control, which would turn on amp-fit-text. |
Thanks for the comments and testing!
Correct, this should be quite straightforward for the
We can probably come back to it later if needed.
Would be great to have amp-fit-text logic indeed, I'll look into it. |
fallbackBackgroundColor, | ||
} = props; | ||
class TextBlockEdit extends Component { | ||
componentDidUpdate() { |
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.
Testing this it feels like maybe it would be good if the user could still assign a custom font size, too.
Note that I didn't make any changes toFontSizePicker
(yet)
At the moment I see two reasonable options here:
- Add a new "Auto" option to font size dropdown *
- Add a "Automatically fit text to container"
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 to helper.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.
Add a "Automatically fit text to container" ToggleControl (like in the post editor) that is enabled by default.
The font size picker would only be displayed when this toggle is disabled.
Good idea, makes it consistent with other options too.
Besides this, I'd probably extract calculateFontSize in its own class method or even move it to helper.js as it is static.
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.
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.
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. 👍
Note that this PR has been developed on the latest |
Regarding 5d24258, can we use the same
Good to know! The next Gutenberg version (5.3) is scheduled to be released on Wednesday, so perhaps these changes are going to be included there. There are some other nice updates in there too, so I'll have a look at updating the dependencies as soon as new packages are being released. |
Just noticed the styling changes now... Odd. Are they really intentional? 🤔 |
In the last iteration of AMP Stories it came out that it was unclear for users the block mover and the dragging functionality exist at all, and it was decided to display the block mover all the time when a block is selected (even when not hovering), for visibility. That's removed by now and is not necessary anymore due to not using the layers, however, maybe for discoverability, it is good to show it when hovering anywhere. Maybe that's a UX detail which we could reiterate on separately though, too. Thoughts?
Which odd changes are you seeing? There was a change for block mover but this should already be addressed in the PR and should display as it was before, are you seeing something different? Could you share a screenshot if you see something off in the PR + |
General changes in the Gutenberg editor; unrelated to this PR. Things like less rounded corners and darker border colors for the block toolbar. Needs some getting used to I guess. |
@miina I was just looking at the code again. Is the width/height here stored in pixels or percentages? For responsiveness I think we need percentages. |
@swissspidy It's in pixels. Hmm, actually the font size is not accurate this way either. |
Actually, |
We can probably try assigning the percentage values to the wrapper and assing |
@swissspidy Would you mind taking another look? The width and height are in % now and also switched to using |
Very neat! The feature seems to work well. There are certainly some styling issues we can address in #1967 to make sure the frontend experience matches the one in the editor as closely as possible. But for now I think this is good. We can always iterate on this in future PRs :-) Oh, and I can't wait for snapping to nicely align elements. That's gonna be 💥. |
Fixes #1965.