Prerequisites for upcoming forms and actions #938

Oksamies · 2023-11-30T21:24:55Z

No description provided.

Open needs to be controllable from outside Header and Dialog z-index was whacky, fix it

packages/ts-api-react-forms/src/ApiForm.tsx

@@ -14,6 +14,7 @@
 > = {
  schema: Schema;
  endpoint: ApiEndpoint<z.infer<Schema>, Result>;
+  metaData: any;


packages/ts-api-react-forms/src/useApiForm.ts

@@ -12,6 +12,7 @@
  Z extends ZodRawShape
 > = {
  schema: Schema;
+  metaData: any;


packages/ts-api-react/src/useApiCall.ts

@@ -3,11 +3,12 @@

 export type ApiEndpoint<Data, Result> = (
  config: RequestConfig,
-  data: Data
+  data: Data,
+  metaData: any


packages/ts-api-react/src/useApiCall.ts

 ) => Promise<Result>;
 export function useApiCall<Data, Result>(
  endpoint: ApiEndpoint<Data, Result>
-): (data: Data) => Promise<Result> {
+): (data: Data, metaData: any) => Promise<Result> {


packages/ts-api-react/src/useApiCall.ts

  const apiConfig = useApiConfig();
-  return (data: Data) => endpoint(apiConfig, data);
+  return (data: Data, metaData: any) => endpoint(apiConfig, data, metaData);


anttimaki · 2023-12-01T10:03:39Z

packages/ts-api-react-forms/src/ApiAction.tsx

+> {
+  schema: Schema;
+  endpoint: ApiEndpoint<z.infer<Schema>, Result>;
+  metaData: any;


What ever happens to this whole file, as per my earlier reviews I would prefer a) better typing b) silencing the warning and adding a comment describing why it was necessary. I like this approach since writing the comment usually (not necessarily in this case) works as rubber ducking, and makes it obvious to others that the choice to use any was deliberate and necessary.

Needless to say, this comment applies to all these warnings.

packages/ts-api-react-forms/src/useApiAction.ts

+  Result extends object,
+  Z extends ZodRawShape
+> = {
+  metaData: any;


anttimaki

Leaving this as a comment since I think we should discuss the two main points of the review before moving forward:

The ApiAction as a concept
Setting default values to form components

anttimaki · 2023-12-01T10:01:21Z

packages/cyberstorm-forms/src/components/FormSelect.module.css

@@ -0,0 +1,64 @@
+.SelectTrigger {


This should use camelCase, not PascalCase, to be consistent with the Cyberstorm package. Applies to other selectors in this files as well.

EDIT: to think of it, these are all used in Select component, so I need no reason to use the Select prefix in the names at all.

Yeah, fixed the classnames

anttimaki · 2023-12-01T10:01:41Z

packages/cyberstorm-forms/src/components/FormSelect.module.css

@@ -0,0 +1,64 @@
+.SelectTrigger {
+  display: inline-flex;


Also generally speaking I feel this file doesn't adhere to all the style things we've set up so far and I'm not sure why you're actively creating code debt by ignoring them. I'm talking about variables for colors, sizes, spaces etc.

Added usage of variables

anttimaki · 2023-12-01T10:01:57Z

packages/cyberstorm-forms/src/components/FormSelect.tsx

+"use client";
+
+import { z, ZodObject } from "zod";
+import { ZodRawShape } from "zod/lib/types";


Per my earlier review, this should be imported from "zod".

anttimaki · 2023-12-01T10:02:04Z

packages/cyberstorm-forms/src/components/FormSelect.tsx

+  // The schema is required to allow TS to infer valid values for the name field
+  schema: Schema;
+  name: Path<z.infer<Schema>>;
+  options: { value: string; label: string }[];


Nice to see you've chosen to use the value/label approach instead of a single string this time around. Although it does make the SelectSear stick out like a sore thumb.

anttimaki · 2023-12-01T10:02:07Z

packages/cyberstorm-forms/src/components/FormSelect.tsx

+  schema: Schema;
+  name: Path<z.infer<Schema>>;
+  options: { value: string; label: string }[];
+  placeholder?: string;


This is never passed to TempSelect?

anttimaki · 2023-12-01T10:03:12Z

packages/cyberstorm/package.json

@@ -28,7 +28,7 @@
    "@radix-ui/react-dialog": "^1.0.2",
    "@radix-ui/react-dropdown-menu": "^2.0.1",
    "@radix-ui/react-radio-group": "^1.1.3",
-    "@radix-ui/react-select": "^1.2.0",
+    "@radix-ui/react-select": "^2.0.0",


IDK how Radix uses semver but is it wise to mix components from v2 with components from v1?

From what I could find, it's alright to mix them.

anttimaki · 2023-12-01T10:03:20Z

packages/cyberstorm/src/components/Dialog/Dialog.module.css

-  transform: translate(calc(var(--left) * -1), calc(var(--top) * -1));
+
+  box-shadow: hsl(206deg 22% 7% / 0.35) 0 10px 38px -10px, hsl(206deg 22% 7% / 0.2) 0 10px 20px -15px;
+  transform: translate(-50%, -50%);


A bit offtopic but is this still the preferred way of centering stuff? I would have thought there's something better by now. @gkurck

Yes, I can use flex 🤷‍♂️

Yes, flex is preferred here, no need to take the element out of the document flow.

anttimaki · 2023-12-01T10:03:25Z

packages/cyberstorm/src/components/Header/Header.module.css

@@ -1,7 +1,7 @@
 .root {
  position: sticky;
  top: 0;
-  z-index: 999;
+  z-index: auto;


Isn't auto the default value? Is this actually needed at all?

anttimaki · 2023-12-01T10:03:33Z

packages/ts-api-react-forms/src/ApiAction.tsx

@@ -0,0 +1,52 @@
+"use client";


This whole thing confuses me. According to commit message, the idea is to "use API without forms". So why is this located in ts-api-react-forms package? Why not ts-api-react, or better yet directly in thunderstore-api?

Secondly, what seems to me to basically be a "onChange" function is for some reason formatted as a React component? Why?

This uses zod, which might be a good idea, but on the other hand might be overkill. It was used on the forms since RHF supports it and helps reduce boilerplate(?). Now were not using RHF. Would regular TS interfaces do the same job?

Also I'm not a fan of the metaData, as a variable name nor as a concept. YMMV but if you compare this to Dapper, everything above thunderstore-api methods treats data as "everything that is needed to make the request, defined by the ts-api method arguments". It's then up to the method to decide what parts of information goes to URL and what goes to query parameters (or request body, although no such use case has been created by me). This way Cyberstorm components don't (need to) know how the data is used, they're just responsible to providing it all.

In summary: I would cut out out ts-api-react-forms/ts-api-react and go either directly to thunderstore-api or possibly through a separate ts-api-actions package. That being said, I feel a lot of the above might be due to me not comprehending the inner workings of ts-api-react-forms/ts-api-react so you might want a second opinion from @MythicManiac

Forgot to elaborate on the naming of metaData: It's one word, so metadata. Even then I'm not sure if it's really "data about the rest of the data". It's additional data, and in this care very much required to form the complete set of "data".

I've moved ApiAction and useApiAction underneath ts-api-react. And added better typing.

The idea that I got from @MythicManiac was that the ApiForm would wrap the usage of RHF and other stuff (like the usage of toasts) to be under one usable component.

I could have added the "actions" just where those were needed, but it seemed like a good idea to add a similar centralized point for the actions as forms have one. Having a bit hard time explaining my reasonings though.

Also about zod; It does feel a bit overkill, but TBH, I'd rather have it than not have it.

Let's continue this discussion in #963

anttimaki · 2023-12-01T10:03:39Z

packages/ts-api-react-forms/src/ApiAction.tsx

+> {
+  schema: Schema;
+  endpoint: ApiEndpoint<z.infer<Schema>, Result>;
+  metaData: any;


What ever happens to this whole file, as per my earlier reviews I would prefer a) better typing b) silencing the warning and adding a comment describing why it was necessary. I like this approach since writing the comment usually (not necessarily in this case) works as rubber ducking, and makes it obvious to others that the choice to use any was deliberate and necessary.

Needless to say, this comment applies to all these warnings.

Improve CreateTeamForm

Add AddTeamMemberForm

Add TeamDetailsEdit form

…form Add AddServiceAccountForm

…nt-form Add RemoveServiceAccountForm

Add DeleteAccountForm

Add DisbandTeamForm

…-change-role Add team member kick and change role

Add TeamLeaveForm

Oksamies added 7 commits November 30, 2023 21:43

Add react-select and update depedencies

b8bfac8

Update Dialog component

d5e0004

Open needs to be controllable from outside Header and Dialog z-index was whacky, fix it

Add FormSelect

9d6cd47

Update FormSubmitButton and FormTextInput

5bf3347

Add metaData to useApiForm, ApiForm and useApiCall

5b71067

Add ApiAction and useApiAction for using API without forms

903a44a

Make FormSubmitButton icon optional

58a06ea

github-advanced-security bot found potential problems Nov 30, 2023

View reviewed changes

Oksamies added 2 commits November 30, 2023 23:28

Improve CreateTeamForm

7becda5

Add AddTeamMemberForm

592c924

Oksamies requested a review from anttimaki December 1, 2023 09:19

anttimaki reviewed Dec 1, 2023

View reviewed changes

anttimaki mentioned this pull request Dec 1, 2023

Add AddTeamMemberForm #940

Merged

Oksamies and others added 16 commits December 4, 2023 18:34

Merge pull request #939 from thunderstore-io/improve-create-team-form

0bb110f

Improve CreateTeamForm

Merge pull request #940 from thunderstore-io/add-add-team-member-form

16e89e7

Add AddTeamMemberForm

Add TeamDetailsEdit form

330ca99

Merge pull request #942 from thunderstore-io/add-team-details-edit

256e98d

Add TeamDetailsEdit form

Add AddServiceAccountForm

b7e73b7

Merge pull request #943 from thunderstore-io/add-add-service-account-…

a62d015

…form Add AddServiceAccountForm

Add RemoveServiceAccountForm

5d57330

Merge pull request #944 from thunderstore-io/add-remove-service-accou…

64f536b

…nt-form Add RemoveServiceAccountForm

Add DeleteAccountForm

d462a18

Merge pull request #945 from thunderstore-io/add-delete-account-form

b8af117

Add DeleteAccountForm

Add DisbandTeamForm

e578e13

Merge pull request #946 from thunderstore-io/add-disband-team-form

d66c0e3

Add DisbandTeamForm

Add team member kick and change role

e287958

Merge pull request #941 from thunderstore-io/add-team-member-kick-and…

9ae6f32

…-change-role Add team member kick and change role

Add TeamLeaveForm

149e962

Merge pull request #947 from thunderstore-io/add-team-leave-form

158f0b3

Add TeamLeaveForm

anttimaki mentioned this pull request Feb 8, 2024

Add all the forms and actions #1029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prerequisites for upcoming forms and actions #938

Prerequisites for upcoming forms and actions #938

Oksamies commented Nov 30, 2023

anttimaki Dec 1, 2023

anttimaki left a comment

anttimaki Dec 1, 2023

Oksamies Dec 8, 2023

anttimaki Dec 1, 2023

Oksamies Dec 8, 2023

anttimaki Dec 1, 2023

anttimaki Dec 1, 2023

anttimaki Dec 1, 2023

anttimaki Dec 1, 2023

Oksamies Dec 8, 2023

anttimaki Dec 1, 2023

Oksamies Dec 8, 2023

gkurck Dec 8, 2023

anttimaki Dec 1, 2023

anttimaki Dec 1, 2023

anttimaki Dec 1, 2023

Oksamies Dec 8, 2023

anttimaki Dec 1, 2023

Prerequisites for upcoming forms and actions #938

Are you sure you want to change the base?

Prerequisites for upcoming forms and actions #938

Conversation

Oksamies commented Nov 30, 2023

Choose a reason for hiding this comment

anttimaki left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment