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 global state to VM create with IT #1284

Merged

Conversation

avivtur
Copy link
Member

@avivtur avivtur commented May 1, 2023

📝 Description

Change Create VM from instance type flow to have a global state to avoid prop drilling and get a more maintainable code.

changes included in this PR:

  • installing zustand - 3rd party lib for state management.
  • moving the create VM from instance type to global state management - please use only in proper places (mostly under the CreateFromInstanceType component with some exceptions)

🎥 Demo

Please add a video or an image of the behavior/changes

@openshift-ci openshift-ci bot requested review from tnisan and upalatucci May 1, 2023 14:00
@openshift-ci openshift-ci bot added the approved This issue is something we want to fix label May 1, 2023
@avivtur avivtur force-pushed the instancetype-vm-context branch 4 times, most recently from ed2bc48 to 89e4a66 Compare May 2, 2023 16:05
@avivtur avivtur force-pushed the instancetype-vm-context branch 3 times, most recently from 104aa7b to 351ffb1 Compare May 3, 2023 08:11
@avivtur avivtur force-pushed the instancetype-vm-context branch 4 times, most recently from 3b71807 to d0c84cc Compare May 3, 2023 09:23
@avivtur avivtur changed the title [WIP] Add context to VM create with IT Add context to VM create with IT May 3, 2023
@avivtur avivtur changed the title Add context to VM create with IT Add global state to VM create with IT May 3, 2023
@hstastna
Copy link

hstastna commented May 3, 2023

@avivtur Was adding another dependency necessary? What are the benefits of zustand comparing to the other solutions? And what is the problem we are trying to solve by this PR? Adding new dependency = a bit worse maintainable project. I am not telling that this is not good, I just want to know more about this PR and related decisions being made. Thanks.

@avivtur
Copy link
Member Author

avivtur commented May 3, 2023

@avivtur Was adding another dependency necessary? What are the benefits of zustand comparing to the other solutions?

Hi @hstastna Thank you for looking :)
Since React has the context API, it's not 100% necesarry. However I've explored a state management solution, and consulting with @metalice we thought zustand has an easy-to-use API and needs less boilerplate code to manage it effectively than the context API, I invite you to explore yourself: https://github.com/pmndrs/zustand
it's also a highly maintained library with a very big community which is also supporting the open-source way :)

And what is the problem we are trying to solve by this PR? Adding new dependency = a bit worse maintainable project. I am not telling that this is not good, I just want to know more about this PR and related decisions being made. Thanks.

as written in the description, I wanted to centralize the state in one place, and avoid prop drilling (components sending props on to child just in the name of passing them as props as well).
adding one more dep to manage the state easily is not making the project less maintainable but the opposite, since we need less code to achieve something.

@metalice
Copy link
Member

metalice commented May 3, 2023

@avivtur Was adding another dependency necessary? What are the benefits of zustand comparing to the other solutions?

Hi @hstastna Thank you for looking :) Since React has the context API, it's not 100% necesarry. However I've explored a state management solution, and consulting with @metalice we thought zustand has an easy-to-use API and needs less boilerplate code to manage it effectively than the context API, I invite you to explore yourself: https://github.com/pmndrs/zustand it's also a highly maintained library with a very big community which is also supporting the open-source way :)

And what is the problem we are trying to solve by this PR? Adding new dependency = a bit worse maintainable project. I am not telling that this is not good, I just want to know more about this PR and related decisions being made. Thanks.

as written in the description, I wanted to centralize the state in one place, and avoid prop drilling (components sending props on to child just in the name of passing them as props as well). adding one more dep to manage the state easily is not making the project less maintainable but the opposite, since we need less code to achieve something.

Another big advantage is that it acts similarly to context, with the option to use multi-store and not central one-big-store, which is similar behavior as context.

@hstastna
Copy link

hstastna commented May 3, 2023

Thanks, guys, for the wider explanation, that's what I was looking for! 👍🏽

Copy link
Member

@pcbailey pcbailey left a comment

Choose a reason for hiding this comment

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

This is a great improvement! It makes things so much cleaner. I only have very minor comments.

Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
@pcbailey
Copy link
Member

pcbailey commented May 4, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label May 4, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avivtur, pcbailey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 4541dc9 into kubevirt-ui:main May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is something we want to fix lgtm Passed code review, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants