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

Update modals and generate meaningful k8s name for images #1529

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Jul 17, 2023

Closes #1433 #1435 #1388 #1326

Description

This PR did the following things:

  1. Add the comment back as mentioned in Refactor BYON images table #1506 (comment) since we are not going to fix it in this phase and I removed it by mistake
  2. Enhance the status toggle of Enable column in the table, for the error images, it will set default to disabled and the user cannot enable it. Also, add the sort to the Enable column as @vconzola requested here Refactor BYON images table #1506 (comment)
Screenshot 2023-07-17 at 4 38 58 PM
  1. Generate a meaning k8s name for the image stream that user imports, before the change, it was byon-${timestamp}, but now it's custom-${translated-k8s-name} where translated-k8s-name is the translation of the display name that user inputs. Also, add a tooltip next to the image name so the user could easily locate the image stream resource on the cluster.
Screenshot 2023-07-17 at 4 45 23 PM
  1. Refactor the Import/Update modal of the custom notebook images, change the labels, and add helper text/tooltips based on the mockups
Screenshot 2023-07-17 at 4 47 59 PM

Highlights: Refactor the table for adding/editing packages and software in the modal. Take the software as an example, when the user starts to add software or edit software, he will be in the edit mode, which only allows the user to confirm or abandon the current change (the user cannot edit multiple lines at the same time and cannot submit or exit the modal).

What's more, when the user hits Enter in the edit mode, it will automatically confirm the current row and move to the next row. If the row is already the last row, it will create a new entry, which helps the user to quickly input and move on without clicking with the mouse every time. When the user hits Esc in the edit mode, it will abandon the current change of the row and exit the edit mode.

Edit mode: close button hidden, Submit and Cancel button disabled, other rows cannot be edited and removed, either
Screenshot 2023-07-17 at 5 02 44 PM
Non-edit mode: You can do anything you want
Screenshot 2023-07-17 at 5 03 10 PM

How Has This Been Tested?

  1. Try to create some images with the correct path (like quay.io/opendatahub/workbench-images:jupyter-minimal-ubi8-python-3.8-pr-89) and incorrect path (like quay.io/opendatahub/workbench-images:invalid-tag) and you could check the status of the toggle on the Enable column. Try to create more and sort by the Enable column
  2. Try to import/edit the image in every situation, play around with the edit mode of the software/packages

Test Impact

TBD

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@DaoDaoNoCode
Copy link
Member Author

@vconzola Could you please check the UX updates in this PR? Thanks!

@vconzola
Copy link

lgtm

Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

Tested all changes on my end, looks fine.

/lgtm

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Some changes needed

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Looking good... let me run a few visual tests. One question...

Comment on lines 48 to 63
>
<FlexItem>{obj.display_name}</FlexItem>
<FlexItem>
<ImageErrorStatus image={obj} />
</FlexItem>
<ResourceNameTooltip resource={convertBYONImageToK8sResource(obj)}>
{obj.display_name}
</ResourceNameTooltip>
<ImageErrorStatus image={obj} />
</Flex>
Copy link
Member

Choose a reason for hiding this comment

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

This seems like you'll not get spacing, why this change?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the magic of Flex, they become flex items by nature and it auto fits them. So although looks weird in code, this works out.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, you're going to run into a conflict -- Gage kept the FlexItems. Perhaps you should match his implementation to avoid conflicts

@andrewballantyne
Copy link
Member

I think this works great overall.

However, we should have a sit down with UX and get feedback on the keyboard actions. I'm not sure I like the escape/enter action flows -- double escape closes the modal without warning. Keyboard flow doesn't really get you back in once you escape out (we should probably focus the "add software" at that point -- but it is the next tab index so it's reasonable)

I think we are already are aware of the issue with a bad image -- I just worry it tosses everything away and it's not just inconvenient for an image name. Which I think now with the added flow of keyboard actions, one could quickly lose more.

Probably useful to incubate these and get feedback across the board on them though.

@openshift-ci openshift-ci bot removed the lgtm label Jul 25, 2023
@DaoDaoNoCode DaoDaoNoCode linked an issue Jul 25, 2023 that may be closed by this pull request
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

FYI

Comment on lines 48 to 63
>
<FlexItem>{obj.display_name}</FlexItem>
<FlexItem>
<ImageErrorStatus image={obj} />
</FlexItem>
<ResourceNameTooltip resource={convertBYONImageToK8sResource(obj)}>
{obj.display_name}
</ResourceNameTooltip>
<ImageErrorStatus image={obj} />
</Flex>
Copy link
Member

Choose a reason for hiding this comment

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

FYI, you're going to run into a conflict -- Gage kept the FlexItems. Perhaps you should match his implementation to avoid conflicts

@andrewballantyne
Copy link
Member

/approve

@DaoDaoNoCode get someone else on the team to verify it again, I unfortunately lost my window to test the code again. Looks good though, thanks for making the desired changes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, manaswinidas

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

@DaoDaoNoCode
Copy link
Member Author

@andrewballantyne Sure, let me squash the commits first.

@DaoDaoNoCode DaoDaoNoCode force-pushed the upstream-issue-1433 branch 2 times, most recently from 10f9b48 to 9750213 Compare July 28, 2023 13:17
@DaoDaoNoCode DaoDaoNoCode force-pushed the upstream-issue-1433 branch 2 times, most recently from a4411a9 to 22aa0ff Compare July 28, 2023 16:57
Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

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

it all works as described. few UX related issues though, not breaking.

  • invalid images will show enabled but be greyed out, you have to refresh for the UI to correct itself and show it to be not enabled and greyed out.
  • when adding software/packages, in edit mode for a row
    1. make edits in one of the text boxes
    2. cancel with either the X button or ESC
    3. edit the row again
    4. the last edits that were canceled are still there

@Gkrumbach07
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 31, 2023
@openshift-merge-robot openshift-merge-robot merged commit 3d68d79 into opendatahub-io:f/byon Jul 31, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants