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

vdk-jupyter: add deploy job component to the UI #1704

Merged

Conversation

duyguHsnHsn
Copy link
Collaborator

@duyguHsnHsn duyguHsnHsn commented Mar 6, 2023

What:
Added the JSX component for deploy job. The component is for creating a deployment not fore removing/updating.
This is how it looks:
Screenshot 2023-03-06 at 14 51 31
Please address everything you think that must be change visually.

PS: Please, give ideas on how to introduce the "Disable", "Enable" flags. You can see them here: https://github.com/vmware/versatile-data-kit/blob/main/projects/vdk-control-cli/src/vdk/internal/control/command_groups/job/deploy_cli.py

Why: Linked to the issue #1282

Tests: jest unit tests

Signed-off-by: Duygu Hasan hduygu@vmware.com

Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

Could you add how is the change tested and verified?

On the question in PR description, how to implement enable/disable,
I think these boolean states (either one or another) are best visualised with an "enabled" checkbox (may default to true).

(nit: then, the value of the checkbox, could additionally be made in sync (fetched) with latest job state at one moment.)

@duyguHsnHsn
Copy link
Collaborator Author

I like the idea about just having Enabled and f it is unchecked to interpret it as Disabled.

@duyguHsnHsn duyguHsnHsn force-pushed the person/hduygu/vdk-jupyter-ui-add-deploy-job-component branch from 20a580c to f52b175 Compare March 6, 2023 12:53
@zverulacis
Copy link
Contributor

Enable / Un-pause might be better
I really don't like un-pause, maybe just Enable is enough and clear enough too.

@antoniivanov
Copy link
Collaborator

Please for name of buttons and other operations, look at what we have in the Operations UI and use the same namings.

@duyguHsnHsn
Copy link
Collaborator Author

Please for name of buttons and other operations, look at what we have in the Operations UI and use the same namings.

Okay, I will do it for deploy but for the other operations shouldn't it be a separate effort?

Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

LVGTM, I like the generalised visual elements are reusable with broader context. For example,
.p-create-job-checkbox becomes .jp-vdk-checkbox since could be reused in every dialog, including but not limited to create.

I looked at the tests, all the tests cover the form fields mapping to the default values expected. In the future, when input validation will be planned, we may need to test the input is also set with the dialog-backing object.

@duyguHsnHsn duyguHsnHsn force-pushed the person/hduygu/vdk-jupyter-ui-add-deploy-job-component branch from 08691c8 to 2ec345f Compare March 7, 2023 17:05
@duyguHsnHsn duyguHsnHsn merged commit ccf725e into main Mar 8, 2023
@duyguHsnHsn duyguHsnHsn deleted the person/hduygu/vdk-jupyter-ui-add-deploy-job-component branch March 8, 2023 12:54
ivakoleva pushed a commit that referenced this pull request Mar 9, 2023
What:
Added the JSX component for deploy job. The component is for creating a
deployment not fore removing/updating.
Why: Linked to the issue
#1282

Tests: jest unit tests

Signed-off-by: Duygu Hasan [hduygu@vmware.com](mailto:hduygu@vmware.com)

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants