Add developer menu #1029

georgefst · 2023-08-14T11:06:46Z

Discussions in #1005 and #1023 about how to toggle experimental features led me to finally look in to something I've long-thought we could do with: an area in the UI for developers only.

We had something similar in the bottom-right corner in Vonnegut, except that IIRC, while that was only intended for development use, it also appeared in the production build. And I can't remember quite what we actually used it for altogether (I think there was an option to dump a JSON serialisation of the current program?).

We already added a dev-only menu in #1002. But that was specifically for React Query. This PR essentially extends that with a bespoke menu for Primer-specific settings.

For convenience in avoiding potential conflicts, this is currently based on #1023.

Feel free to bikeshed. This is currently just a quick idea without much thought given to the design. Although I haven't marked as a draft, since I do consider it to be in a mergeable state.

brprice · 2023-08-14T11:52:11Z

src/App.tsx

  useEffect(() => {
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
-    window.toggleDevtools = () => setShowDevtools((old) => !old);
+    window.toggleDevtools =
+      // This comment forces a line break to limit the scope of `@ts-ignore`.
+      () => setEnableDevtools((old) => !old);
  }, []);


What's going on here with the linting stuff?

Oh yeah, it is kind of unrelated, but it's an improvement. @ts-ignore is a very big hammer. So without this change, even, say, a typo in the name of setEnableDevtools wouldn't be flagged.

And, to be clear, the only error we want to ignore is Property 'toggleDevtools' does not exist.

dhess · 2023-08-14T11:54:37Z

I think this is a good idea, and I like the new ability to view node IDs, but I think "always show labels" should be a student-visible preference, or at least another tree/text button toggle state until we have a proper preference system, and not fobbed off into the developer tools menu; so this doesn't solve my objection to #1023.

brprice · 2023-08-14T11:59:58Z

src/App.tsx

+                      <input
+                        type="checkbox"
+                        id="showIDs"
+                        onChange={(e) => setShowIDs(e.target.checked)}
+                        className="mr-1"
+                      />
+                      <label htmlFor="showIDs">show node IDs</label>


You mention in your commit message that

Note that, for now at least, we only show these for the "primer", "primer-simple" and "primer-box" nodes. For others, it's less obvious where to put them, since we don't have labels as such.

Some off-the-cuff ideas on where to put them if we want:

mouse over/hover text

some sort of popup on click

click toggles display of normal node or just its id

in the sidebar or info display when node is selected

Having said that, I don't currently see a pressing need for them. Maybe leave it as-is until there is a situation where we want to see them?

FTR, one (painful!) way to access these IDs is to inspect the node in your browser's devtools and then find the data-id attribute on the containing react-flow__node node. (At least, I think this works, I haven't tested it much)

brprice · 2023-08-14T12:03:57Z

src/App.tsx

-    window.toggleDevtools = () => setShowDevtools((old) => !old);
+    window.toggleDevtools =


It might be nice to explicitly say (in the commit message) something to the effect that the "new" devtools are accessed the same way as the "old" react-query ones? (At least, I had forgotten how to access the tools prior to this pr!)

I suppose it isn't obvious if you've only explored this via the PR deployment, rather than locally, but in development builds there's also a button in the top-right of the UI, similarly to before:

Oh, actually, ignore me, that probably is obvious. I'd forgotten that running window.toggleDevtools() only enables that button, rather than directly opening the panel.

I had also forgotten how to enable this! Just a mention in the commit message would be fine.

georgefst · 2023-08-14T12:26:02Z

I think "always show labels" should be a student-visible preference, or at least another tree/text button toggle state until we have a proper preference system, and not fobbed off into the developer tools menu

What if we switch the default to "always show labels", so that #1023 has no actual end-user-visible effects?

The idea was that we can also use this for temporary features which we haven't found a place for in the UI, rather than just stuff that will always be developer-facing. This avoids blocking the likes of #1023 on unspecified future UI work.

dhess · 2023-08-14T12:30:39Z

What if we switch the default to "always show labels", so that #1023 has no actual end-user-visible effects?

Yes, that would be my preference, and then I would have no objection either to #1023 or this PR. See #1023 (comment) for my reasoning.

georgefst · 2023-08-15T14:55:45Z

Expert-mode label hiding is now off by default.

One more thought before I re-request a review: should I put #1005 behind this menu as well, instead of the ugly tree 1/tree 2 split (which was only chosen because at the time we really had nowhere else to put the option)?

dhess · 2023-08-15T14:59:27Z

Thanks!

I suppose it makes sense to do the same with inline labels, but it may not be worth the effort. IMO, both hiding labels and inline labels should be available to the student, as it would be interesting to observe their preferences during testing. So before we start serious testing, we'll want to add a proper preferences UI, anyway, where both of those options could go.

georgefst · 2023-08-15T15:45:40Z

I suppose it makes sense to do the same with inline labels, but it may not be worth the effort.

Wasn't much effort. I've just pushed this change.

src/App.tsx

dhess · 2023-08-16T09:07:48Z

src/App.tsx

+    </div>
+  </div>
+);
+


I would prefer that this were a separate component, not inlined in App, but I can take care of that refactoring later.

This retains the ReactQuery devtools panel, but also has an extra area to the right (unused right now) in which we can add further controls. We can use this for gating features which are experimental or which help with debugging. In production builds, this can be triggered as before, by running `window.toggleDevtools()` in a browser console. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

One allows us to display node IDs, which can be useful for debugging. Note that, for now at least, we only show these for the "primer", "primer-simple" and "primer-box" nodes. For others, it's less obvious where to put them, since we don't have labels as such. And they'd be less useful anyway, since the mentioned node types happen to be the ones whose IDs are backend-generated. The other allows us to enable the expert-mode syntax-label-hiding feature. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

This makes it easier to see where dev opts are set. We also control the checkbox state, making it easier to set an initial value to `true`, should we ever wish to. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

Signed-off-by: George Thomas <georgefsthomas@gmail.com>

The button needs to be above the sessions page panel. And the others are actually unnecessary now that we introduce scrollbars to the main canvas instead of overlaying it. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

georgefst requested a review from a team August 14, 2023 11:06

github-actions bot deployed to primer-app (Preview) August 14, 2023 11:09 View deployment

georgefst added the Deploy service Deploy the backend service for this PR label Aug 14, 2023

brprice reviewed Aug 14, 2023

View reviewed changes

brprice approved these changes Aug 14, 2023

View reviewed changes

dhess mentioned this pull request Aug 14, 2023

Add option to hide non-syntax labels in expert mode #1023

Merged

georgefst force-pushed the georgefst/dev-tools branch from 01c5b34 to 74b3f0e Compare August 15, 2023 14:39

github-actions bot deployed to primer-app (Preview) August 15, 2023 14:40 View deployment

github-actions bot deployed to primer-app (Preview) August 15, 2023 15:46 View deployment

georgefst requested a review from dhess August 16, 2023 08:42

georgefst force-pushed the georgefst/dev-tools branch from 1efb83b to fade269 Compare August 16, 2023 08:52

github-actions bot deployed to primer-app (Preview) August 16, 2023 08:54 View deployment

dhess approved these changes Aug 16, 2023

View reviewed changes

georgefst added 5 commits August 16, 2023 14:53

refactor: Factor out DevMenu component

c6ffb6a

This makes it easier to see where dev opts are set. We also control the checkbox state, making it easier to set an initial value to `true`, should we ever wish to. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

feat: Move "inline labels" setting from toolbar to dev menu

7f0301b

Signed-off-by: George Thomas <georgefsthomas@gmail.com>

fix: Fix devtools z-indices

aac4a7b

The button needs to be above the sessions page panel. And the others are actually unnecessary now that we introduce scrollbars to the main canvas instead of overlaying it. Signed-off-by: George Thomas <georgefsthomas@gmail.com>

georgefst force-pushed the georgefst/dev-tools branch from fade269 to aac4a7b Compare August 16, 2023 13:54

georgefst enabled auto-merge August 16, 2023 13:54

github-actions bot deployed to primer-app (Preview) August 16, 2023 13:56 View deployment

georgefst added this pull request to the merge queue Aug 16, 2023

Merged via the queue into main with commit 9e2e873 Aug 16, 2023
7 checks passed

georgefst deleted the georgefst/dev-tools branch August 16, 2023 14:00

georgefst mentioned this pull request Nov 22, 2023

Expose internal Ids in "raw"/"debug" projection? #62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add developer menu #1029

Add developer menu #1029

georgefst commented Aug 14, 2023

brprice Aug 14, 2023

georgefst Aug 14, 2023

georgefst Aug 14, 2023

dhess commented Aug 14, 2023 •

edited

Loading

brprice Aug 14, 2023

brprice Aug 14, 2023

georgefst Aug 14, 2023

georgefst Aug 14, 2023

dhess Aug 14, 2023

georgefst commented Aug 14, 2023

dhess commented Aug 14, 2023 •

edited

Loading

georgefst commented Aug 15, 2023

dhess commented Aug 15, 2023

georgefst commented Aug 15, 2023

dhess Aug 16, 2023

		window.toggleDevtools = () => setShowDevtools((old) => !old);
		window.toggleDevtools =

Add developer menu #1029

Add developer menu #1029

Conversation

georgefst commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhess commented Aug 14, 2023 • 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

georgefst commented Aug 14, 2023

dhess commented Aug 14, 2023 • edited Loading

georgefst commented Aug 15, 2023

dhess commented Aug 15, 2023

georgefst commented Aug 15, 2023

Choose a reason for hiding this comment

dhess commented Aug 14, 2023 •

edited

Loading

dhess commented Aug 14, 2023 •

edited

Loading