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: Change root FS position for path parameter #2550

Merged
merged 17 commits into from
Sep 11, 2023

Conversation

gageorgiev
Copy link
Contributor

@gageorgiev gageorgiev commented Aug 14, 2023

Currently, VDK commands in Jupyter require a path parameter which points to the location of the data job relatively to the current location of the user in the Jupyter file system menu on the right.

This is suboptimal, as it makes it less consistent to use and is the cause of at least one bug.

This change streamlines this by making it so the path parameter is always assumed to point relatively to the FS location where Jupyter was instantiated. This means all commands will expect the same path param value for a given job, regardless of where the user is currently located within the Jupyter FS menu.

This also fixes a bug where a job inside a given directory cannot be ran from inside that directory.

Some UI changes were also introduced, now the placeholder for the path contains the whole Jupyter path and the input is adjusting to the size of the placeholder (since we put the whole relative path it can become a long string)

Screenshot 2023-09-11 at 10 05 47

This UI change gives hint to the user what we expect for the input and makes it proof for user mistakes.

Testing done: only manual so far, working on an actual test

Currently, VDK commands in Jupyter require a path parameter which
points to the location of the data job relatively to the current
location of the user in the Jupyter file system menu on the right.

This is suboptimal, as it makes it less consistent to use and is the
cause of at least one bug.

This change streamlines this by making it so the path parameter is
always assumed to point relatively to the FS location where Jupyter
was instantiated. This means all commands will expect the same path
param value for a given job, regardless of where the user is currently
located within the Jupyter FS menu.

This also fixes a bug where a job inside a given directory cannot be
ran from inside that directory.

Testing done: only manual so far, working on an actual test

Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
…on/gageorgiev/jupyter-default-selected-button

Signed-off-by: Gabriel Georgiev <gageorgiev@vmware.com>
@gageorgiev gageorgiev marked this pull request as ready for review August 17, 2023 12:13
Copy link
Collaborator

@duyguHsnHsn duyguHsnHsn left a comment

Choose a reason for hiding this comment

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

Looks good! Please make sure that nothing in the front-end remains that works with the previous implementation!
For example does this cover the use-case od default job-input which fetches the current directory from the server and if the user does not modify it, it uses it? I am not exactly sure how it was working but please check it. If it seems all fine I will approve the pr

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

Let's merge it.

I am still not confident adjustInputWidth logic is worth the cost (dev, maintenance, performance) but it's an usability improvement and I am all for that. So we will evaluate as we see how it's used and if it causes issues.

@duyguHsnHsn duyguHsnHsn enabled auto-merge (squash) September 11, 2023 08:55
@duyguHsnHsn duyguHsnHsn merged commit 8bfe625 into main Sep 11, 2023
7 of 8 checks passed
@duyguHsnHsn duyguHsnHsn deleted the person/gageorgiev/jupyter-default-selected-button branch September 11, 2023 09:09
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

5 participants