Skip to content

feat: implement "Revert" button for workspace name #276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Feb 11, 2025

Conversation

kantord
Copy link
Member

@kantord kantord commented Feb 7, 2025

2025-02-10.16-36-54.mp4

This PR is a draft, currently containing an implementation of a few small improvements on the workspace settings page (based on #269 and consultation with James). But the biggest part of the work is refactoring regarding forms.

In general I invested way too much time into this PR, albeit none of that is (hopefully wasted). Before continuing with the PR, I would like to ask some feedback on my plan for proceeding.

In the current state, not all the features work perfectly and I think it's not wise for me to invest more time in fixing them with the current implementation.

My plan for proceeding

I would like to undo my changes to the currently broken feature (model preference) and merge this PR as it is, with the jsonforms based implementation. Afterwards I would like to remove the jsonforms based implementation again, but keep the other abstractions, and finish the remaining works (=currently broken features) without jsonforms.

I also don't plan to add any other forms library just yet for pragmatic reasons, since the current solution should work perfectly with the abstractions I introduced in this PR.

My reasoning:

  • The effort required to work with jsonforms cannot be justified at the moment
  • this PR is already big enough, and my proposed mergable version will contain useful abstractions + avoid some merge conflicts with other works. It will also technically improve user experience and only temporarily include an inconsistency
  • my original insistence on trying jsonforms for this purpose was based on my previous negative experiences with other form libraries and dissatisfaction with their design philosophy. But this experiment with jsonforms produced some similar negative feelings

I will try to be concise in the detailed summary of what I learned, but I want to keep a record of the learnt information even if it won't be used right now. Feel free to skip the parts you don't care about.

In the end I think that both of you had the correct intuitions about the scenario, and kinda me too, but I was blinded by previous negative experiences with other libraries 😹

Details

The good

jsonforms, in theory is very well designed and extremely easy to use. Also it seems compatible with our

component without any problems, and I believe that it should be compatible with some low-level react form libraries, as long as those allow setting/reading the form state as a single value.

I also found a library that allows defining JSON schema in a very convenient way, using a zod-like syntax:

const schema = Type.Object({
  workspaceName: Type.String({
    title: "Workspace name",
    minLength: 1,
  }),
});

The library also provides a generic type, which similar to zod, converts the schema into a type. This way there is a very clear binding between the types/state and the UI, with very convenient type safety.

In the grand scheme of things, I think this is an extremely convenient way of creating a form in react:

const schema = Type.Object({
  workspaceName: Type.String({
    title: "Workspace name",
    minLength: 1,
  }),
});

export function WorkspaceName(props: Props) {
  // definition of submit handler

  return (
    <FormCard
      schema={schema}
      initialData={initialData}
      onSubmit={handleSubmit}
    />
  );
}

The bad

I realized that there is a certain learning curve to jsonforms. I would say that building the first 2-3 custom form fields is quite difficult because you have to understand some of the inner design/quirks/unusual abstractions of jsonforms. For instance, it has a separate concept for controls and cells. Cells are "bare" form elements (such as a dropdown menu) while controls are fully independent form controls such as a checkbox with a label. It is in theory possible to build a generic control that only contains a label and a slot for a cell, or you can choose to build a component that directly contains the label and the input field as well.

In theory, this is a diminishing cost, so in the grand scheme of things it is basically irrelevant. It's diminishing both because of learning about the library and because the more already pre-built components we have, the more chance there is that it works out of the box after only writing the schema. The problem is that we don't have a lot of pre-built components yet, so in the upcoming weeks this would be a drag on productivity.

And I think that with our current strategy, we cannot afford this temporary drag on productivity.

The ugly

The major downside of the json schema based form management is that it completely takes over the validation system. For a stable product, this is a blessing in disguise, since actively designing the forms around this system would minimize complexity, maintenance costs and bugs with an acceptable cost in user experience.

But since we are not in "maintenance mode" we care a lot more about how the product will look in a demo, so probably we are fine with a few bugs and unnecessary complexity here and there while we prioritize the complete customization of error messages instead of automatically generated ones popping up everywhere.

Working around the schema I think could in theory also be a blessing in disguise for UX work in a stable product, but in our case this would just add additional workload to James in the short term, again something not acceptable in our current situation

@ghost
Copy link

ghost commented Feb 7, 2025

Minder Vulnerability Report ✅

Minder analyzed this PR and found it does not add any new vulnerable dependencies.

Vulnerability scan of 5f42a30d:

  • 🐞 vulnerable packages: 0
  • 🛠 fixes available for: 0

@coveralls
Copy link
Collaborator

coveralls commented Feb 7, 2025

Pull Request Test Coverage Report for Build 13261729647

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 57 of 76 (75.0%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 68.226%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/forms/rerenders/renderChildren.tsx 4 5 80.0%
src/forms/rerenders/utils.tsx 8 9 88.89%
src/forms/rerenders/controls/Checkbox.tsx 4 6 66.67%
src/forms/rerenders/controls/EnumField.tsx 6 10 60.0%
src/forms/rerenders/ObjectRenderer.tsx 4 9 44.44%
src/mocks/msw/handlers.ts 9 15 60.0%
Totals Coverage Status
Change from base Build 13259375783: -0.5%
Covered Lines: 868
Relevant Lines: 1189

💛 - Coveralls

@kantord kantord changed the title Improve user experience workspcaes Improve user experience on workspaces page Feb 10, 2025
@kantord kantord marked this pull request as ready for review February 10, 2025 15:39
@kantord kantord requested review from peppescg and alex-mcgovern and removed request for peppescg February 10, 2025 15:39
@peppescg
Copy link
Collaborator

peppescg commented Feb 10, 2025

love the title details sections 😆

@peppescg
Copy link
Collaborator

My feeling is that the power of JSONForms here is too much ( I see the value when you have a schema from BE, and you only need to pass to the form, but if we need to manually create the schema I guess we loose one important benefit from json form, having a schema based form without adding any piece of code ), and considering the effort of adding those components that we're missing in Minder, I guess it could be overwhelming as you also said. I would use a simple hook form, react-hook-form that is well connected with RAC, using zod for enforcing the schema type when it is needed.

FYI: I am refactoring a lot the preferred model section, cause we are introducing rules...

@kantord kantord force-pushed the improve-user-experience-workspcaes branch from f33c695 to 29dc62b Compare February 11, 2025 11:14
@kantord kantord changed the title Improve user experience on workspaces page feat: implement "Revert" button for workspace name Feb 11, 2025
@kantord kantord enabled auto-merge (squash) February 11, 2025 11:43
@kantord kantord merged commit 7e66ff0 into main Feb 11, 2025
7 checks passed
@kantord kantord deleted the improve-user-experience-workspcaes branch February 11, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants