Update Console Components #3276

raclim · 2024-11-13T20:41:50Z

Related Issues:

Fixes Console history not accessible with up arrow #3271
Progress towards Javascript error in running this sketch #3268
(Not an issue, also just makes additional changes to Console.jsx)

Changes:

moves the styling for the Console into a separate utility file, consoleStyles.js
separates hook in ConsoleInput.jsx into individual ones to handle keyboard inputs for Enter, ArrowUp, and ArrowDown
removes named import in previewEntry.js to prevent "_report is undefined" warnings

I have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. Fixes #123

release-com · 2024-11-13T20:49:47Z

Release Environments

This Environment is provided by Release, learn more!
To see the status of the Environment click on Environment Status below.

p5.js-web-editor
- app-ted3fd0.release.editor.p5js.org/
- preview-ted3fd0.release.editor.p5js.org/

🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-861652405e

lindapaiste

previewEntry.js needs to be fixed.

ConsoleInput.jsx looks pretty solid but could be cleaner. I'll see if I can write a PR with what I have in mind.

lindapaiste · 2024-11-13T22:37:31Z

client/utils/previewEntry.js

@@ -175,6 +176,6 @@ if (_report) {
    paths.forEach((path) => {
      newMessage = newMessage.replaceAll(path, window.objectPaths[path]);
    });
-    _report.apply(window.p5, [newMessage, method, color]);
+    p5Call.apply(window.p5, [newMessage, method, color]);


Previously the apply was on window.p5._report (a function). Now it is on window.p5 (an object). So that's a mistake. I would keep a _report variable but define it inside the if (p5Call) branch.

Circling back to this, perhaps the only change that we need in this file is from const { _report } = window.p5; to const _report = window.p5?._report;.

However I do have questions as to why window.p5 is undefined in the first place. I'll leave some more info on the issue.

Thanks for looking further into this!! I actually didn't realize there were so many issues about this already 😂

I think for now, only changing window.p5 to window.p5?.report makes sense to me.

I'll change this PR as progress towards the issue, since it seems like we need to change where previewScripts is loading, but I'm not sure where the best placement might be for it yet.

lindapaiste · 2024-11-13T22:39:33Z

client/modules/IDE/components/Console.jsx

  useEffect(() => {
-    const unsubscribe = listen(handleMessageEvent);
-    return function cleanup() {
-      unsubscribe();
-    };
-  });
+    listen(handleMessageEvent);
+  }, [handleMessageEvent]);
+


I would keep the unsubscribe on cleanup behavior.

lindapaiste · 2024-11-13T22:43:17Z

client/modules/IDE/components/Console.jsx

+            dispatchConsoleEvent={(event) =>
+              dispatch(ConsoleActions.dispatchConsoleEvent(event))
+            }


IMO it's better to do the dispatching directly from ConsoleInput.

lindapaiste · 2024-11-13T22:44:50Z

client/modules/IDE/components/ConsoleInput.jsx

+    return () => {
+      if (cmInstance.current) {
+        cmInstance.current = null;
+      }
+    };


Probably don't need a cleanup here if all that it's doing is clearing a local variable. If there's a destroy or cleanup function on the CM instance we would want to call that though.

lindapaiste · 2024-11-13T22:49:57Z

client/modules/IDE/components/ConsoleInput.jsx

+      cmInstance.current.on('keydown', handleEnterKey);
+    }
+
+    return () => {
+      if (cmInstance.current) {
+        cmInstance.current.off('keydown', handleEnterKey);
+      }
+    };
+  }, [commandHistory, dispatchConsoleEvent]);


The off and on is good. That will prevent us having multiple event handlers.

Having dispatchConsoleEvent as a dependency means that this effect will re-run on every render of Console because the function is declared inline and is not memoized. You could memoize it (with useCallback) but I would instead handle the dispatching here instead of passing a function prop.

lindapaiste · 2024-11-13T22:51:06Z

client/modules/IDE/components/ConsoleInput.jsx

+        cm.setValue(commandHistory[newCursor] || '');
+        const cursorPos = cm.getDoc().getLine(0).length - 1;
+        cm.getDoc().setCursor({ line: 0, ch: cursorPos });


Good change ✔️

lindapaiste · 2024-11-13T22:54:49Z

client/modules/IDE/components/ConsoleInput.jsx

-    cmInstance.current.getWrapperElement().style['font-size'] = `${fontSize}px`;
-    cmInstance.current.refresh();
-  }, [theme, fontSize]);
+  }, [commandCursor, commandHistory]);


We def need these as dependencies if we want to use those variables, but this does mean that we are changing the handler very frequently. The goal with having both values in the same state is that you can access both values from within a prevState callback. So the the effect doesn't need to re-run on every change of the variables.

Yeah I think this was what I started to become unsure about as I was working on it 😅

lindapaiste · 2024-11-14T20:20:51Z

client/modules/IDE/utils/consoleStyles.js

+        style
+      );
+    default:
+      return '';


This is supposed to return an object, right? So return ''; doesn't make sense.

Yes!! I'm thinking of setting the default to the light theme instead?

lindapaiste · 2024-11-14T20:22:35Z

client/modules/IDE/components/ConsoleInput.jsx

 ConsoleInput.propTypes = {
  theme: PropTypes.string.isRequired,
-  dispatchConsoleEvent: PropTypes.func.isRequired,
  fontSize: PropTypes.number.isRequired
 };


theme and fontSize are both selected from Redux. so those can be selected inside the ConsoleInput rather than selected by the parent and passed down.

Although now I see that the parent still needs to select them regardless because it uses them for the ConsoleFeed style.

lindapaiste · 2024-11-14T20:37:23Z

client/modules/IDE/components/ConsoleInput.jsx

+    cmInstance.current.getWrapperElement().style['font-size'] = `${fontSize}px`;
+  }, []);
+
+  useEffect(() => {
+    if (cmInstance.current) {
+      cmInstance.current.setOption('theme', `p5-${theme}`);
+      cmInstance.current.getWrapperElement().style[
+        'font-size'
+      ] = `${fontSize}px`;


I think we can add a style property to one of the divs that we control instead of manipulating the element.

<div ref={codemirrorContainer} className="console__editor" style={{ fontSize: `${fontSize}px` }} />

Where it is being set currently is a direct child of that div but it has the same effect.

Just tried it out!

raclim · 2024-11-18T15:35:29Z

I'm going to merge this in since I noted in the original issue that I would get something out for it soon, but am definitely open to following up with further changes here!

bojidar-bg · 2024-11-19T15:22:40Z

client/modules/IDE/components/ConsoleInput.jsx

        });
+
        dispatchConsoleEvent(consoleEvent);


Wouldn't you need to wrap this in dispatch = useDispatch(), dispatch(...), like the reducers used in Console.jsx? Asking since, currently, while Up arrow does work, the console no longer echoes the inputted commands. (:

Yes!! Thank you so much for catching that 😂 I thought I had it in at some point! I'll try to have the update out for this at some point early next week.

The update works, thanks a ton! 🎉

raclim added 3 commits November 9, 2024 19:23

separate console styles into another file

12ff655

remove named import

f99a3e2

separate key handlers into different hooks

66e8994

remove typo

54903cb

lindapaiste requested changes Nov 13, 2024

View reviewed changes

raclim added 4 commits November 14, 2024 13:23

edit cleanup functions, dispatchConsoleEvent in ConsoleInput

8178b92

change back report variable, make value conditional

1e4c4d5

remove extra space

24490ec

fix typo

4e674cb

lindapaiste reviewed Nov 14, 2024

View reviewed changes

raclim and others added 3 commits November 14, 2024 15:45

set light as default console theme

0912253

add style to console editor div, remove styling hook

a850d5c

Merge branch 'develop' into update-console

0a7c9d9

raclim merged commit 7104c7f into develop Nov 18, 2024
4 checks passed

bojidar-bg reviewed Nov 19, 2024

View reviewed changes

raclim deleted the update-console branch November 27, 2024 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Console Components #3276

Update Console Components #3276

raclim commented Nov 13, 2024 •

edited

Loading

release-com bot commented Nov 13, 2024

lindapaiste left a comment

lindapaiste Nov 13, 2024 •

edited

Loading

lindapaiste Nov 14, 2024

raclim Nov 14, 2024

lindapaiste Nov 13, 2024

lindapaiste Nov 13, 2024

lindapaiste Nov 13, 2024

lindapaiste Nov 13, 2024

lindapaiste Nov 13, 2024

lindapaiste Nov 13, 2024 •

edited

Loading

raclim Nov 14, 2024

lindapaiste Nov 14, 2024

raclim Nov 14, 2024

lindapaiste Nov 14, 2024

lindapaiste Nov 14, 2024

lindapaiste Nov 14, 2024

raclim Nov 14, 2024

raclim commented Nov 18, 2024

bojidar-bg Nov 19, 2024

raclim Nov 22, 2024

bojidar-bg Dec 12, 2024

Update Console Components #3276

Update Console Components #3276

Conversation

raclim commented Nov 13, 2024 • edited Loading

release-com bot commented Nov 13, 2024

Release Environments

lindapaiste left a comment

Choose a reason for hiding this comment

lindapaiste Nov 13, 2024 • edited Loading

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

lindapaiste Nov 13, 2024 • edited Loading

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

raclim commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raclim commented Nov 13, 2024 •

edited

Loading

lindapaiste Nov 13, 2024 •

edited

Loading

lindapaiste Nov 13, 2024 •

edited

Loading