Teacher tool: notes area auto resizing on enter, more text entered #9920

srietkerk · 2024-03-16T01:17:32Z

While a user is typing in the notes field, we want the textarea to grow with the text input so they don't have to deal with cropping and scrolling.

The kind of interesting thing that happens right now is that when you change tabs, the expanded style of the text box goes away, but once you make changes in it, the expanded style of the textarea returns. I kind of like this effect, but if others think it feels like a bug, I can investigate persisting the style changes between state changes.

Also, right now, I am getting an error from the iframe driver seemingly from a setHighContrast event with trying to turn something into a string when it's undefined. I stepped through it in the debugger, and couldn't find anything wrong in the first run-through, but there might be something happening because of strict mode. This error doesn't cause any problems for interacting with the app, but is something we should fix. I can look into it more on Monday.

Upload target: https://makecode.microbit.org/app/c75e983d88131448e2408ef0f2839596db1eff2e-3521ca26a9--eval

thsparks · 2024-03-18T19:13:23Z

I think the min size is good, but I also think that when we do start expanding, we're leaving too much buffer below. We probably don't need to start expanding until we reach the 3rd line.

eanders-ms · 2024-03-18T21:24:33Z

teachertool/src/components/DebouncedTextarea.tsx

@@ -11,6 +11,8 @@ export interface DebouncedTextareaProps extends TextareaProps {
 export const DebouncedTextarea: React.FC<DebouncedTextareaProps> = ({ intervalMs = 500, ...props }) => {
    const timerId = useRef<NodeJS.Timeout | undefined>(undefined);
    const latestValue = useRef<string>("");
+    const textareaRef = useRef<HTMLTextAreaElement>(null);


Just a note of caution about the use of useRef here: if these refs change for any reason, the values captured in the useEffect below will go stale (even adding the refs as dependencies won't change this). If this happens, control resizing will break.

It's probably unlikely to happen in the current use case, but wanted to note it. A safer way to allocate the state the ref slot is to use useState, and then add the value as a dependency to the useEffect.

thsparks · 2024-03-19T17:40:33Z

teachertool/src/components/DebouncedTextarea.tsx

@@ -21,11 +23,28 @@ export const DebouncedTextarea: React.FC<DebouncedTextareaProps> = ({ intervalMs
    // If the timer is pending and the component unmounts,
    // clear the timer and fire the onChange event immediately.
    useEffect(() => {
+        const resizeObserver = new ResizeObserver(() => {


I don't think I quite follow why this is necessary in addition to the text area resizing. Could you break it down for me?

It also assumes (I think) that all instances of the DebouncedTextarea will want autoResize, which may not be the case. So if this is necessary, we should probably make it optional just like we did in textarea.

Nope, you're right, this isn't needed. I thought I needed this for the resizing to work nicely with the parent component, but it was just css stuff that I needed to change, and since I changed the css stuff after I added the logic to DebouncedTextarea, I thought they were both needed. Removed!

thsparks · 2024-03-19T17:41:16Z

react-common/components/controls/Textarea.tsx

@@ -57,6 +61,10 @@ export const Textarea = (props: TextareaProps) => {
        if (onChange) {
            onChange(newValue);
        }
+        if (autoResize && resizeRef.current) {


What happens if someone says autoResize = true but doesn't pass in a resizeRef?

Yeah, I was thinking about this, too. It felt weird to me, though, that just passing a resizeRef would just immediately make the textarea resizable. As I'm typing this, though, I think I was just being a bit overcautious. I think just having the resizeRef is fine, and that can be the source of truth for knowing that a we want the textarea to auto-resize.

I do not think we should have resizeRef implicitly cause resizing to start happening. I actually think we should rename resizeRef to just textareaRef (or get rid of the parameter if possible). But what happens if autoResize is true and the ref is not passed in? Nothing?

Oh, I hadn't tested that, but just did. If you don't pass in a ref, you get a bunch of errors. I think I see what you're saying, though. We shouldn't need to pass in a ref, and should only need the flag because the ref is just the HTML textarea. Let me play with it.

Got rid of it as a prop, now just defining the ref in the component.

thsparks · 2024-03-19T17:41:47Z

react-common/components/controls/Textarea.tsx

@@ -38,6 +40,8 @@ export const Textarea = (props: TextareaProps) => {
        readOnly,
        resize,
        wrap,
+        autoResize,
+        resizeRef,


Maybe just textareaRef? People could use this for all sorts of things, not just resizing.

thsparks

Nice. One more nitpick, and I still would like if we could reduce the bottom padding a bit, but nothing blocking.

thsparks · 2024-03-19T21:52:49Z

teachertool/src/components/DebouncedTextarea.tsx

@@ -39,5 +39,5 @@ export const DebouncedTextarea: React.FC<DebouncedTextareaProps> = ({ intervalMs
        timerId.current = setTimeout(sendChange, intervalMs);
    };

-    return <Textarea {...props} onChange={onChangeDebounce} />;
+    return <Textarea {...props} autoResize={true} onChange={onChangeDebounce} />


Since DebouncedTextareaProps extends TextareaProps, someone can actually set autoResize when specifying a DebouncedTextarea component (i.e. <DebouncedTextarea autoResize={true} ... />), which I think is a good thing.

So instead of hardcoding this to true for all DebouncedTextAreas, could we let it pass-through as a part of the {...props} and just set autoResize to true in the Notes component?

srietkerk · 2024-03-19T23:24:09Z

I still would like if we could reduce the bottom padding a bit, but nothing blocking.

So, this is something that I can do, but it's kinda tricky. I've been playing with it a bit, and just using this:
textareaRef.current.style.height = ``${textareaRef.current.scrollHeight}px``; actually gets the non-padded result.

However, I unfortunately can't just get rid of textareaRef.current.style.height = "1px";. This is needed for when you're getting rid of lines and making the text area smaller. Setting the height to the smaller value and then setting it to the scroll height makes the component realize the correct scroll height (because without this line, the textarea will just stay at the largest scrollHeight/height achieved).

Similarly, I can't have

   textareaRef.current.style.height = "1px";
   textareaRef.current.style.height = `${textareaRef.current.scrollHeight}px`;

because when you're just inputting text for the first time, the combination will shrink the textarea to smaller than the height we have for it originally.

By putting some console logs, I was able to see that the original scrollHeight of the textarea is 64px. So, I can add a check for the scrollHeight being greater than what the scrollHeight is originally to make sure that we don't get the undesired shrinking if we haven't inserted any new lines, and this will give the desired behavior:

    if (textareaRef.current.scrollHeight > 64) {
        textareaRef.current.style.height = "1px";
    }
    textareaRef.current.style.height = `${textareaRef.current.scrollHeight}px`;

The tricky part of this is that 64 pixels is the need for the teacher tool. If there is a different original height for the textarea, having 64 as the comparison number will likely introduce undesired size changes. So, in order to make it so there is no padding at the bottom, we would need to know the scroll height at first render. I can play with using a ref for this, but we'd need to make sure that the textareaRef is defined first, so there might be weirdness with timing. We could also have a prop for scrollHeightMinimum, but we'd want to have a default value for it if it's not defined, so it might get messy.

…textarea for notes

thsparks · 2024-03-20T06:14:26Z

because when you're just inputting text for the first time, the combination will shrink the textarea to smaller than the height we have for it originally.

Would setting a min-height in the css solve this problem?

srietkerk · 2024-03-20T18:07:23Z

because when you're just inputting text for the first time, the combination will shrink the textarea to smaller than the height we have for it originally.

Would setting a min-height in the css solve this problem?

Yep, that's what it needed. Thank you!

notes area auto resizing on enter, more text entered

0f07886

srietkerk requested a review from a team March 16, 2024 01:17

eanders-ms approved these changes Mar 18, 2024

View reviewed changes

thsparks reviewed Mar 19, 2024

View reviewed changes

srietkerk added 2 commits March 19, 2024 14:32

do not need a resize observer in debouncedtextarea

6e6fe22

remove ref as a prop, using use ref in textarea instead

0b8183f

thsparks approved these changes Mar 19, 2024

View reviewed changes

moved autoresize to criteriaresultsnotes, when initializing debounced…

84be19b

…textarea for notes

srietkerk added 2 commits March 20, 2024 10:20

merged master, resolved conflicts

f6c0186

more accurate resizing, got rid of unneeded padding

0f53e31

srietkerk merged commit e62d76c into master Mar 20, 2024
6 checks passed

srietkerk deleted the srietkerk/tt-notes-resize branch March 20, 2024 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teacher tool: notes area auto resizing on enter, more text entered #9920

Teacher tool: notes area auto resizing on enter, more text entered #9920

srietkerk commented Mar 16, 2024

thsparks commented Mar 18, 2024

eanders-ms Mar 18, 2024

thsparks Mar 19, 2024

thsparks Mar 19, 2024

srietkerk Mar 19, 2024

thsparks Mar 19, 2024

srietkerk Mar 19, 2024

thsparks Mar 19, 2024

srietkerk Mar 19, 2024

srietkerk Mar 19, 2024

thsparks Mar 19, 2024

thsparks left a comment

thsparks Mar 19, 2024

srietkerk commented Mar 19, 2024

thsparks commented Mar 20, 2024

srietkerk commented Mar 20, 2024

Teacher tool: notes area auto resizing on enter, more text entered #9920

Teacher tool: notes area auto resizing on enter, more text entered #9920

Conversation

srietkerk commented Mar 16, 2024

thsparks commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thsparks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srietkerk commented Mar 19, 2024

thsparks commented Mar 20, 2024

srietkerk commented Mar 20, 2024