-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Switch react-autosize-textarea to react-textarea-autosize #48215
Conversation
Size Change: -4.01 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in 9f254c0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4554910986
|
I happened to be working on a similar problem in #39619.
However, as noted in this comment, I will be happy to help with testing! |
I tested it on Windows. Unless my procedure was incorrect, the problem did not reproduce. Is this an OS issue_? 87403cc7ee654e487a825da29fd3e9a9.mp4 |
I have tested both Chrome and Firefox on Windows OS.
As for the In the two blocks using PlainText (HTML Block, Search Block), I found the following problems: Switching to the code editor and back to the normal editor. Height will be incorrect. Occurs only in Chrome. Screencast for the Custom HTML BLock161b1b9a25d30445cf1b94e3d4781c4f.mp4Screencast for the Shortcode Blockd5b38fedb6ac1b8ce5dab905f1ee2422.mp4Switching to the code editor for the first time, the HTML will cut off. Occurs only in Firefox. Screencast for the Custom HTML Block76d979c0e3d496ba8378c14b54056236.mp4Screencast for the Shortcode Block03397d1a02c5d7d81cc729480b43c533.mp4As for the React Native app, I can't test it, but the version 1 PlainText component is used in the following three places: gutenberg/packages/block-library/src/code/edit.native.js Lines 41 to 53 in b2c16f3
gutenberg/packages/block-library/src/file/edit.native.js Lines 484 to 488 in b2c16f3
gutenberg/packages/block-library/src/search/edit.native.js Lines 306 to 327 in b2c16f3
|
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.
The code changes themselves look well to me. There are API differences between those two packages, but we're not using any of those APIs, so we're safe on that side.
It seems like the only problem we need to take care of is the regression that came from manual testing - kudos @t-hamano for the thorough testing 🙌
It could be -- I'm on Firefox in macOS. However, the issues you noted sound very similar to that one :) In your first video, I don't think there is enough text in the editor to trigger the bug (it has to be longer than the default minimum height) Here's my video of the problem: https://user-images.githubusercontent.com/6265975/219794503-2f3282f2-0395-4839-a3e7-f7dc115d0131.mp4 It sounds like we are blocked until we can get the maintainer to release a new version :( There is a proposed fix (Andarist/react-textarea-autosize#353), which might solve these problems for us. However, it doesn't look like the maintainer has been active in the past few weeks. I don't think we can reasonably fix the bug on our side |
You're right, I was also able to reproduce this problem in Windows OS.
If you don't mind, could you test if this problem reproduces on MacOS as well? The proposed fix (Andarist/react-textarea-autosize#353) appears to be for firefox, so even if a fixed version is released, this issue might not be resolved. |
The fix was merged in that repo, so hopefully it'll make it to npm in the next day or two, at which point I'll revisit this and test that scenario! |
v8.4.1, which fixes bugs related to Firefox, has been released. How about incorporating this package into this PR and testing it again? |
0af83e3
to
9f254c0
Compare
I tried this PR again with the latest version of I cannot determine which side this problem is on, but if you click on the |
Update: A new PR has been submitted on this issue in the |
Nice, thanks for the update! |
Update: I saw a tweet suggesting that the native textarea element might auto-adjust its height. In the future, if this is supported by major browsers, we may be able to solve this problem without relying on libraries 😊 |
Btw, we could completely simulate a textarea with a Ideally we should deprecate PlainText version 1, as said earlier, so that's one use of textarea less.
Yes, we should fix that (I can't remember the reasons there) |
I'm going to close the PR -- the fixes still haven't been shipped to the library, and I won't have time to work on this any more. Plus, Ella's idea sounds more ideal |
What?
Solves at least part of #48009
The package we use to auto-resize text areas in React doesn't support React 18 and hasn't been actively maintained for 3 years. Another package, react-textarea-autosize does support React 18, so this PR attempts to switch.
One issue I found so far is that the initial text height is incorrect in some situations. I could only verify this bug in Firefox, not Chrome or Safari.
Not sure why, though. 🤔 Reported the bug here: Andarist/react-textarea-autosize#366
Why?
We want to upgrade to modern npm/node versions, which means resolving peer dependency errors. As a result, we need to use maintained packages which have peer dependencies on React 18 instead of older React versions.
How?
Just switching the packages!
Testing
This seems to be mostly working, but I'm noticing that the text area does not re-size on initial load. It does resize after reloading the page, for some reason.
We need to test these components:
BlockHTML
component inblock-editor
(used in the block list in "html" mode)PlainText
component inblock-editor
CodeEditorTextArea
component inedit-site
PostTextEditor
component ineditor
Testing Instructions for Keyboard
Screenshots or screencast