show all existing PVCs as dropdown under volumes for a new notebook server #5086
Conversation
|
This change is |
|
@avdaredevil , please let me know if you can review this PR as well? |
There was a problem hiding this comment.
Small nits, looks good otherwise, PTAL @kimwnasptd if you can?
/lgtm
components/jupyter-web-app/frontend/src/app/resource-form/volume/volume.component.html
Outdated
Show resolved
Hide resolved
components/jupyter-web-app/frontend/src/app/resource-form/volume/volume.component.ts
Outdated
Show resolved
Hide resolved
|
/lgtm |
|
@avdaredevil thanks for taking a look! @lalithvaka I'll take a more thorough look at this asap. |
|
@kimwnasptd Can you please review this? If you have any additional changes, I will submit the pull request. Am wondering if this can be pushed to 1.1 release as well! |
|
@kimwnasptd , any progress on this? Thank you. |
|
@lalithvaka my only objection is about removing the inputs since the delete button looks too spaced out in the screenshots. the code is LGTM also from me |
|
@kimwnasptd , I have tested this feature with our end user community. They all liked the dropdown but they got confused on the "Size" and "Mode" fields. They were all assuming that any existing PVC they pick, they are expecting the fields disabled are reflection of what the actual size and mode of the selected PVC which is not the case. Hence to remove the confusion, I got rid of the fields on the selection of Existing PVCs. I totally agree with you that the spacing of the delete button needs some adjustment. Please let me know if you can approve or merge this? |
|
Moving ahead on this in favor of getting it in 1.1 Thanks for making this change @lalithvaka! /cc @jlewi |
|
/approve |
|
@lalithvaka Looks like there are test failures |
|
/retest |
1 similar comment
|
/retest |
|
/test kubeflow-presubmit |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avdaredevil, jlewi 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 |
|
/lgtm |
|
@lalithvaka: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@jlewi , @kimwnasptd , @avdaredevil , pre submit test failures are fixed. Can you please lgtm / approve this PR? I would like to submit a cherry pick for this into 1.1. Thank you. |
|
/lgtm |
…ver table #5086: show all existing PVCs as dropdown under volumes while Cherry pick of #5074 #5086 on v1.1-branch. #5074: added gpu details to the notebook server table #5086: show all existing PVCs as dropdown under volumes while (#5162) * added gpu details to the notebook server table * Added a distint function to process GPU details * Removing the stale pre submit tests * show all existing PVCs as dropdown under volumes while creating a notebook server * show all existing PVCs as dropdown under volumes while creating a notebook server
|
@lalithvaka there is an issue with this if you type the name of a notebook which has existed in past, the UI gets a bit confused, and changes to "existing" but leaves the "name" field as text field, rather than a dropdown. |
|
@thesuperzapper That was the behavior before I submitted the PR to show the drop down list for the existing PVCs. I thought about how to address that but couldn't come up with a better option with out totally revamping the existing design. So I left it as it just shows the user that name is an existing PVC. I am not a UX/UI expert, if you think of anything better, please go ahead and submit an additional PR. |
…erver (kubeflow#5086) * show all existing PVCs as dropdown under volumes while creating a notebook server * show all existing PVCs as dropdown under volumes while creating a notebook server * Removing the stale pre submit tests
…erver (kubeflow#5086) * show all existing PVCs as dropdown under volumes while creating a notebook server * show all existing PVCs as dropdown under volumes while creating a notebook server * Removing the stale pre submit tests
Resolves issue #4883
This PR enhances two things
Follow are the screenshots of before and after
Before the fix
After the fix
