Skip to content
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

Refactor state management #25

Merged
merged 12 commits into from
Apr 15, 2024
Merged

Conversation

oliverroick
Copy link
Collaborator

To implement state management, I did a fair bit of refactoring and simplification of the code. Main changes include:

  • Introduces index.jsx as the entry point for the form, to separate mounting the app to the DOM from the UI components. I added a new component ProfileForm for rendering the overall form
  • Simplified the form components
    • Introduced two new components ImageSelect and ResouceSelect so we can separate functionality required for image selection only from the more generic resource selection.
    • Both components are still driven by the config, however the two special image-select cases, select docker image and build new image, are now configured in the front-end to increase clarity in the code. Before the change one was configured and the config, the other in the front-end, which added unnecessary complexity. (I’m open to hear your thoughts on this, I’m aware that I might have limited context here.)
    • Abstracted converting the config to form choices into useSelectOptions so it can be used across the various form components.

I completely revamped the state management for the app to create a single source of truth that is accessible from all components below the context.

  • Moved selected profile in SpawnerFormProvider so it’s accessible through out.
  • I removed the reducer because we only need to track a small number of variables in state, and using a reducer creates unnecessary overhead.

Open questions:

  • The original solution had a canSubmit flag, presumably as a simple way of form validation. I’d suggest we implement form validation and error messaging as well.

const [repo, setRepo] = useState("");
const [builtImage, setBuiltImage] = useState(null);
const { costumImage, setCustomImage } = useContext(SpawnerFormContext);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo - costumImage -> customImage ?

@yuvipanda
Copy link
Member

Ty @oliverroick.

I'm OOO rest of the week, but I've invited @batpad to this repo so he should be able to review + merge as appropriate.

The original solution had a canSubmit flag, presumably as a simple way of form validation. I’d suggest we implement form validation and error messaging as well.

Correct, and the jankiness of figuring out 'when should the start button be enabled?' is what made me look for refactoring the state management.

@oliverroick
Copy link
Collaborator Author

Addressed the feedback from @batpad and added a couple more changes:

  • Added form validation and error display.
  • Refactored form-field composition, to reduce boilerplate when adding labels, fields and errors.

@batpad
Copy link
Collaborator

batpad commented Apr 11, 2024

@oliverroick - this is looking great and I think we should merge this.

Some minor things:

The validation / error messaging and all the state management clean-ups are looking really great to me! Let's discuss and if it helps to move things forward, let's merge this, else we can wait for @yuvipanda to be back next week and see if he has any comments.

@batpad
Copy link
Collaborator

batpad commented Apr 14, 2024

@oliverroick noticed the prettier check failed on the last commit :( - sorry, could you fix that and let's merge this. Thanks!

@batpad batpad merged commit b918aa2 into 2i2c-org:main Apr 15, 2024
1 check passed
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.

3 participants