Apply actions #617

georgefst · 2022-11-10T12:14:47Z

We can now finally build programs again!

There is one bug I've noticed here: the actions list doesn't go back to empty when the selected node is deleted. The problem is that we never get the new selection from the backend. So the frontend still thinks it has a selection, but it doesn't correspond to any node in the tree. This leads to errors in the console if we then try to apply an action, due to a bad request. So the backend API.Prog needs to be modified to contain the Selection. Note that once this is fixed, initial selection here should be set from the backend initial prog, instead of being undefined.

Out of scope for this PR: rendering prompts, showing explanation when options list is empty, setting level (currently hardcoded to expert), logging (we silently show an empty list when available actions call fails, mutateAsync can also error).

georgefst · 2022-11-10T15:51:49Z

src/components/ActionInput/index.tsx

+};
+
+export const ActionInput = (p: ActionInputProps): JSX.Element => {
+  const [input, setInput] = useState<string>("");


Seems a bit boilerplate-y, but AFAICT this is the recommended approach. Eventually we may want to send "is this acceptable input" requests on every keystroke anyway.

georgefst · 2022-11-10T15:53:32Z

src/components/ActionInput/index.tsx

+  onSubmit: (option: string) => void;
+};
+
+export const ActionInput = (p: ActionInputProps): JSX.Element => {


A lot of this code is taken from #307. We can probably DRY some of it when we rebase that PR.

georgefst · 2022-11-10T15:54:40Z

src/components/ActionPanel/index.tsx

+
+type State =
+  | { state: "ActionList" }
+  | { state: "Input"; action: InputAction; opts: Options };


We'll probably add something like a "PrimitiveInput" state when we hook up #307. Or indeed separate states for each primitive.

src/Actions.tsx

georgefst · 2022-11-10T16:01:44Z

src/components/ActionInput/index.tsx

+          type="button"
+          onClick={(_) => p.onSubmit(input)}
+        >
+          <ArrowUturnLeftIcon className="scale-y-[-1] w-6"></ArrowUturnLeftIcon>


This isn't a perfect match for the arrow in the Figma designs. But I couldn't find one at https://heroicons.com/.

It is a bit hidden in figma, but it is a (rotated) hero icon

Although this seems to have been removed in v2. See https://v1.heroicons.com/ and search for "reply" (I can't see how to put a direct link)

Aha, thanks - I just searched for "arrow"

...and clearly didn't scour the document thoroughly enough.

Although this seems to have been removed in v2.

Ah, I missed that bit. I'll keep this as is then, until anyone has a better idea.

Although this seems to have been removed in v2.

Ah, I missed that bit. I'll keep this as is then, until anyone has a better idea.

Is there any reason not to use the v1 version?

I didn't see anything for "reply" in the react library we're using, so I assume we're already on v2. Though we could easily manually render the old SVG, if we want.

so I assume we're already on v2

Yes: there's "@heroicons/react": "^2.0.13" in package.json.

Yes, "reply" became "arrow-uturn-left" in the 2.0 release:

tailwindlabs/heroicons#750

Unfortunately, "arrow-uturn-left" is rendered substantially differently and looks more appropriate for undo than submit.

We might want to simply drop the icon. I don't think anyone will wonder what to do with an input field. I think the reason we added it in the first place was to distinguish between a placeholder value and a value that the student has typed, but I can't recall for certain. (There are other ways to distinguish between the placeholder and actual input: a greyed out and italicized placeholder font, for example.)

Yes, "reply" became "arrow-uturn-left" in the 2.0 release:

tailwindlabs/heroicons#750

Thanks. It is also mentioned in the release notes, located at https://github.com/tailwindlabs/heroicons/releases/tag/v2.0.0

georgefst · 2022-11-10T16:11:14Z

src/components/ActionPanel/index.tsx

+                          )}
+                          onClick={(_) => onOption(option)}
+                        >
+                          {option.option}


We render global names as unqualified here. See #603.

georgefst · 2022-11-10T16:18:47Z

src/App.tsx

      ) : (
-        <ActionButtonList actions={[]} />
+        <div className="p-10">
+          Click something on the canvas to see available actions!


This text is really a placeholder for something that we've never really discussed. What should we show in the actions panel when there's no selection?

Perhaps we shouldn't show the panel at all, and allow the canvas to naturally extend further right? Though this may be a bit weird since we'd expect that there will "almost always" be a selection during normal use.

Another option is to modify the backend so that selection can never be undefined. We could set initial selection to the first definition (though "first" is pretty arbitrary), and ensure that when the selected node is deleted, the selection always moves somewhere (maybe this part is actually already the case - we don't yet get selection from the backend - see OP).

Before this PR, we showed the list with no actions. I changed that here, because a lot more plumbing is now needed to create an actions list. In particular, passing around click handlers for buttons which, in this case, don't even exist.

Our programs will always have at least one hole, so maybe just ensure that something is always selected.

(We couldn't strictly enforce this: the server might be down and not reply with the program, or there could be a bug, etc. But Maybe we just don't worry about those cases and show an empty panel (or an error message).)

We also have plans, if we haven't already implemented it, to record the cursor location in the backend state, so ensuring that something is always selected should be easy enough to do.

It would be nice to keep the same background even if there are no actions.

The implementation in this commit is fine for this PR, but let's definitely fix this up later. At the very least, we can show the action list background and display a "No actions available" message, or something like that. It should be extremely rare for the reasons mentioned above.

cloudflare-workers-and-pages · 2022-11-10T17:14:31Z

Deploying with Cloudflare Pages

Latest commit:	`b32fa79`
Status:	✅ Deploy successful!
Preview URL:	https://a581b1db.primer-app.pages.dev
Branch Preview URL:	https://georgefst-actions.primer-app.pages.dev

View logs

dhess · 2022-11-14T14:12:15Z

src/Actions.tsx

+  action: NoInputAction | InputAction
+): { font?: string; text: string } => {
+  const code = (text: string) => {
+    return { font: "font-code", text };


I've started using this technique for choosing Tailwind classes, as well, but one drawback is that we lose some static linting checks. Thoughts?

I think it's worth it. Though it is pretty maddening that we lose those checks any time we want to extract some class names in to a variable. We already have this problem all over the place, particularly in ActionButton.

Ideally, TS would be smart enough to treat class names as a separate type to string, and thus ESLint would be able to work out where to apply its checks based on type information. Not that I see that happening.

Maybe the Tailwind people will come up with a solution for this. It has to be a very common issue.

dhess · 2022-11-14T15:04:38Z

I think our Chromatic plan has restarted, so I'm going to re-enable the checks, and force-rebuild this PR in Buildkite.

Now that we no longer get this data from the backend, we have the freedom to make this code simpler and more idiomatic.

We also remove some comments about `ActionPanel` in `src/components/ActionButton/ActionButton.stories.tsx`. We've had the button list for a while, including its own stories, so there's nothing remaining to do. We now elide a background in the `ActionButton` stories so that we don't have to worry about it falling out of sync and being misleading as to the appearance of `ActionPanel`.

This fixes a typo from 6b4d724.

georgefst commented Nov 10, 2022

View reviewed changes

This was referenced Nov 10, 2022

Bump Primer and adapt to available actions API rewrite #620

Merged

(Consistently) display module prefix on global names #603

Open

feat: UI element - Insert primitive values #307

Closed

georgefst force-pushed the georgefst/actions branch from c6d9633 to a0b9526 Compare November 10, 2022 17:13

github-actions bot deployed to Preview (64c77722) November 10, 2022 17:14 View deployment

georgefst requested a review from a team November 14, 2022 12:58

dhess reviewed Nov 14, 2022

View reviewed changes

dhess approved these changes Nov 14, 2022

View reviewed changes

georgefst added 7 commits November 14, 2022 15:18

refactor: Remove unnecessary nullary arguments in button list stories

501cfcf

refactor: Remove ActionName type

576fcfa

Now that we no longer get this data from the backend, we have the freedom to make this code simpler and more idiomatic.

feat: Indicate when actions list is empty due to empty selection

a01e929

feat: Add ActionInput component

475b729

feat: Add the ability to apply actions from the action panel

8ae4a86

fix: Don't mark MakeCase and MakeAPP as destructive

8cf5188

georgefst force-pushed the georgefst/actions branch from a0b9526 to 8cf5188 Compare November 14, 2022 15:19

github-actions bot deployed to Preview (7459a7d2) November 14, 2022 15:20 View deployment

fix: Use type application in Storybook type actions example

b32fa79

This fixes a typo from 6b4d724.

georgefst added the Ready to merge Ready to merge label Nov 14, 2022

github-actions bot deployed to Preview (a581b1db) November 14, 2022 16:23 View deployment

mergify bot added a commit that referenced this pull request Nov 14, 2022

Merge of #617

ab91c28

mergify bot mentioned this pull request Nov 14, 2022

merge-queue: embarking main (e02a0ac) and #617 together #632

Closed

4 tasks

mergify bot merged commit 5ce07c7 into main Nov 14, 2022

mergify bot deleted the georgefst/actions branch November 14, 2022 16:32

georgefst mentioned this pull request Nov 14, 2022

Make Prog selection non-Maybe hackworthltd/primer#773

Closed

georgefst mentioned this pull request Dec 7, 2022

Use selection from backend Prog #675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply actions #617

Apply actions #617

georgefst commented Nov 10, 2022 •

edited

Loading

georgefst Nov 10, 2022

georgefst Nov 10, 2022

georgefst Nov 10, 2022 •

edited

Loading

georgefst Nov 10, 2022

brprice Nov 10, 2022

georgefst Nov 10, 2022

georgefst Nov 10, 2022

georgefst Nov 14, 2022

brprice Nov 14, 2022

georgefst Nov 14, 2022

georgefst Nov 14, 2022

dhess Nov 14, 2022

brprice Nov 14, 2022

georgefst Nov 10, 2022

georgefst Nov 10, 2022

georgefst Nov 10, 2022

dhess Nov 10, 2022

dhess Nov 10, 2022

brprice Nov 10, 2022

dhess Nov 14, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 10, 2022 •

edited

Loading

dhess Nov 14, 2022

georgefst Nov 14, 2022

dhess Nov 14, 2022

dhess commented Nov 14, 2022

Apply actions #617

Apply actions #617

Conversation

georgefst commented Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgefst Nov 10, 2022 • 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

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

dhess Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Nov 10, 2022 • edited Loading

Deploying with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhess commented Nov 14, 2022

georgefst commented Nov 10, 2022 •

edited

Loading

georgefst Nov 10, 2022 •

edited

Loading

dhess Nov 14, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 10, 2022 •

edited

Loading