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

Add profile select #30

Merged
merged 11 commits into from
May 6, 2024
Merged

Conversation

oliverroick
Copy link
Collaborator

This PRs main goal is to add profile selection to the profile form. In the progress of implementing the proposed design, I made some fundamental changes to the profile form architecture.

  • Forms are now dynamically generated based to the configuration. In the process I removed ImageSelect and turned the ResoourceSelect into a generic component to render profile options
  • Each form component now manages its own value state and validation. We’re relying on platform functionality to submit the form and prevent the from from submitting when fields are invalid.

@yuvipanda yuvipanda requested a review from batpad April 26, 2024 15:01
@batpad
Copy link
Collaborator

batpad commented May 6, 2024

Sorry for the delayed review here - had some kerfuffle with my local minikube setup that took a bit of wrangling to figure out :-/

@oliverroick THIS IS LOOKING SO GOOD! I tested multiple profiles, a few different options, it all seems to work really well!

@yuvipanda I have a feeling this is going to make you quite happy :-) . There might be minor things to discuss around the layout and things, I suggest we do that later / ideally when someone can give the whole thing a bit of design thinking.

@oliverroick could you look at the prettier issues? Once those are fixed, I'd be very much in favour of merging this to go ahead with adding some tests, etc.

@batpad
Copy link
Collaborator

batpad commented May 6, 2024

@oliverroick ah sorry, I noticed a minor issue -

Steps to reproduce:

  • Don't select any of the profile radio buttons, try and submit form.
  • Correctly, it shows a validation error: "Select a container profile"
  • Now, select any profile radio

Expected:

  • Validation error should go away, now that a radio is selected

Actual:

  • The validation error saying "Select a container profile" remains even after I have selected a radio.

I think there's two options here:

  • Select the first profile option radio by default, so there is always one of the radios selected, so we don't run into this situation
  • Of course, or fix the validation so that once a radio input is selected the "Select a container profile" validation error goes away

@batpad
Copy link
Collaborator

batpad commented May 6, 2024

@oliverroick any idea why this is still failing? https://results.pre-commit.ci/run/github/633634408/1714976827.7lLMyqY9Tp-3-1zrtfUllQ - do we need some eslint config or something?

@batpad
Copy link
Collaborator

batpad commented May 6, 2024

hmm @oliverroick I see that if I try and run pre-commit run --all locally, this also now fails on main, so I don't think the failure has anything to do with your commits in this branch - not fully sure what's up.

I'll poke around a bit.

@batpad batpad merged commit 6da7b06 into 2i2c-org:main May 6, 2024
3 checks passed
@batpad batpad deleted the profile-select-2 branch May 6, 2024 12:11
@yuvipanda
Copy link
Member

wheee this is exciting :) Thanks @oliverroick and @batpad!

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