MR Permissions - Group and Project dropdown issues #3219

YuliaKrimerman · 2024-09-18T14:47:11Z

RHOAIENG-12025

Description

Verify that once you've selected an item in the dropdown, if you then open the dropdown, you can NOW focus the typeahead text field without the need to close the dropdown( it was blocking the user from doing it) - try typing something in the typeahead in addition to the selected option
The Option in the bottom of the dropdown list is now Select and not Create
After you choose an option in the drop down, try deleting it in the typeahead and verify that it's not creating duplicate entries of that field in the dropdown(see screenshot in the issue)

How Has This Been Tested?

Tested on the UI

Test Impact

One test change to update the Create to Select . Also When changing the label to select I didn't change all the variable names of that label so they are still mostly shown as "create ...." in the code and not as select (except at the original line)

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[x

] The developer has added tests or explained why testing cannot be added (unit or cypress tests 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

YuliaKrimerman · 2024-09-18T14:48:24Z

frontend/src/components/TypeaheadSelect.tsx

@@ -88,7 +88,7 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
  noOptionsFoundMessage = defaultNoOptionsFoundMessage,
  isCreatable = false,
  isCreateOptionOnTop = false,
-  createOptionMessage = defaultCreateOptionMessage,
+  createOptionMessage = defaultSelectOptionMessage,


I didn't propagate the change of this name variable into the code. Let me know if it's needed ( I started but then decided that it will just be a bunch of places instead of just a label change, as we need it on the UI)

We should still call it defaultCreateOptionMessage so we're not disrupting things too much in here, we just need to pass a different value for createOptionMessage where we render this component in RoleBindingPermissionsNameInput.

codecov · 2024-09-18T15:29:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.49%. Comparing base (b5351a7) to head (186b5df).
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3219      +/-   ##
==========================================
+ Coverage   85.39%   85.49%   +0.09%     
==========================================
  Files        1277     1280       +3     
  Lines       28082    28229     +147     
  Branches     7487     7558      +71     
==========================================
+ Hits        23980    24133     +153     
+ Misses       4102     4096       -6

Files with missing lines	Coverage Δ
frontend/src/components/TypeaheadSelect.tsx	`66.47% <100.00%> (+0.75%)`	⬆️
...ts/roleBinding/RoleBindingPermissionsNameInput.tsx	`96.29% <100.00%> (+0.14%)`	⬆️
...pts/roleBinding/RoleBindingPermissionsTableRow.tsx	`89.83% <ø> (ø)`

... and 32 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5351a7...186b5df. Read the comment docs.

mturley

A few things here @YuliaKrimerman . There's one thing I'd love to get @jeff-phillips-18 's opinion on - and if we can't get that settled within the week we should probably exclude it from this issue and open a followup issue for just that focus bug that isn't a MR blocker.

mturley · 2024-09-18T21:53:27Z

frontend/src/components/TypeaheadSelect.tsx

@@ -69,7 +69,7 @@ export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSe
 }

 const defaultNoOptionsFoundMessage = (filter: string) => `No results found for "${filter}"`;
-const defaultCreateOptionMessage = (newValue: string) => `Create "${newValue}"`;
+const defaultSelectOptionMessage = (newValue: string) => `Select "${newValue}"`;


@YuliaKrimerman we need to not change this for all consumers of TypeaheadSelect, only our usage in RoleBindingPermissionsNameInput. Can you please undo this change and instead use the createOptionMessage prop to override it in that usage only?

mturley · 2024-09-18T21:54:33Z

frontend/src/components/TypeaheadSelect.tsx

@@ -88,7 +88,7 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
  noOptionsFoundMessage = defaultNoOptionsFoundMessage,
  isCreatable = false,
  isCreateOptionOnTop = false,
-  createOptionMessage = defaultCreateOptionMessage,
+  createOptionMessage = defaultSelectOptionMessage,


We should still call it defaultCreateOptionMessage so we're not disrupting things too much in here, we just need to pass a different value for createOptionMessage where we render this component in RoleBindingPermissionsNameInput.

mturley · 2024-09-18T22:04:45Z

frontend/src/components/TypeaheadSelect.tsx

@@ -248,12 +246,17 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
  };

  const onTextInputChange = (_event: React.FormEvent<HTMLInputElement>, value: string) => {
-    setFilterValue(value || '');
+    setFilterValue(value);


I'm curious if removing this was necessary?

mturley · 2024-09-18T22:04:51Z

frontend/src/components/TypeaheadSelect.tsx

+    if (selected && value !== selected.content) {
+      // Clear the selection when the input value changes
+      if (onSelect) {
+        onSelect(undefined, '');
+      }
+    }


This seems possibly undesirable, the selection shouldn't change until the user clicks another option. I assume this was to fix the value being duplicated when there's a selected option that has a space and you type in the input? I wonder if there's another way to fix that.

Actually @YuliaKrimerman I think we should remove these lines, I troubleshooted a little and I think maybe the problem is outside this component. If you console.log selectOptions in RoleBindingPermissionsNameInput you'll see that actually when that option gets duplicated it is really being added in the list of options we pass down here. So it's something strange we're doing out there. It's also very strange that it only seems to happen for items that have a space in them.

I think it has something to do with these lines in RoleBindingPermissionsNameInput:

if (value && !typeAhead.includes(value)) { selectOptions.push({ value, content: value }); }

Ah -- it's because of the namespaceToProjectDisplayName thing we do there. When you select a project that includes spaces, that's a display name like "savitha MR test", which is what ends up in our selectOptions and is selected as our value there. But the typeAhead array is actual project names, like "savitha-mr-test". So the above lines are triggering adding the option to the list because it thinks it's missing.

I think the fix is to change the above line to:

if (value && !selectOptions.some((option) => option.value === value)) { selectOptions.push({ value, content: value }); }

This is how it's behaving now when I implement it with the suggested fix. It does that and then after a few seconds it wipes out those funny duplicates and behave normally

Addressed all other comments and kept the original implementation for this, let me know if it's ok to keep it, or if we want to take out this issue for the future.

@YuliaKrimerman with your other change in place to remove the selectOptions.push code, I tried removing the lines you added here (clear the selection when the input value changes) and it seems to behave like I would expect. I'm not seeing the above duplicates. Can you try removing this now and see if it works for you?

mturley · 2024-09-18T22:07:45Z

frontend/src/components/TypeaheadSelect.tsx

      openMenu();
-      setTimeout(() => {
-        textInputRef.current?.focus();
-      }, 100);
-    } else if (isFiltering) {
-      closeMenu();
    }
+    setTimeout(() => {
+      textInputRef.current?.focus();
+    }, 100);
  };


This change seems reasonable to me and does fix the focus issue, but I'm curious if @jeff-phillips-18 has an opinion on it since he wrote this code in #3094.

Looking at this, I would say it is ok to remove the timeout, but in the <Select> component below, add parameter:

shouldFocusFirstItemOnOpen={false}

This will keep focus on the input when the user clicks.

@jeff-phillips-18 what about the removal of the closeMenu() call here? I don't think I'm seeing the menu close when clicking the input anyway, I wonder if that's getting counteracted in the existing code. I don't see a problem with this part of the diff as-is, just wanted to make sure.

YuliaKrimerman · 2024-09-19T18:22:29Z

frontend/src/components/TypeaheadSelect.tsx

@@ -401,6 +403,7 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
      onSelect={handleSelect}
      onOpenChange={(open) => !open && closeMenu()}
      toggle={toggle}
+      shouldFocusFirstItemOnOpen={false}


Added as suggested from Jeff

YuliaKrimerman · 2024-09-19T18:23:03Z

frontend/src/concepts/roleBinding/RoleBindingPermissionsNameInput.tsx

-  if (value && !typeAhead.includes(value)) {
-    selectOptions.push({ value, content: value });
-  }
-


Removed the code that was adding the current value to selectOptions if it wasn't in typeAhead. This ensures that custom items don't persist in the dropdown.

Ah fair point, I guess removing this entirely solves that as well

YuliaKrimerman · 2024-09-19T18:26:01Z

frontend/src/concepts/roleBinding/RoleBindingPermissionsNameInput.tsx

@@ -68,6 +64,18 @@ const RoleBindingPermissionsNameInput: React.FC<RoleBindingPermissionsNameInputP
        }
      }}
      placeholder={placeholderText}
+      createOptionMessage={(newValue) => `Select "${newValue}"`}
+      filterFunction={(filterValue, options) => {


Added this filterFunction in order to achieve the requirement It filters the existing options based on the input and adds a new option with the exact input value if no matches are found.

I don't see any different behavior here when I try removing this filterFunction. With or without it, I'm actually seeing that if I click the new option that doesn't match any select items, it doesn't remain selected in the dropdown. I think that's because we still need it to be one of the selectOptions passed above for it to show as selected.

I also notice that with this filterFunction in place, if you type text that doesn't even partially match any other options, it gets shown as the sole option without the "Select ___" text. So I think we don't want this filterFunction and we need to fix the issue some other way.

@YuliaKrimerman I think I figured it out - Try this. Remove this filterFunction, and above where you removed the other code, add this:

// If we've selected an option that doesn't exist via isCreatable, include it in the options so it remains selected if (!selectOptions.some((option) => option.value === value)) { selectOptions.push({ value, content: value }); }

That ensures that our "newly created" item is in the list once it is selected, but disappears from the list if it is deselected.

mturley

Sorry for the roundabout comments, I have a solution for the problem I identified here:

mturley · 2024-09-19T21:01:17Z

frontend/src/concepts/roleBinding/RoleBindingPermissionsNameInput.tsx

@@ -68,6 +64,18 @@ const RoleBindingPermissionsNameInput: React.FC<RoleBindingPermissionsNameInputP
        }
      }}
      placeholder={placeholderText}
+      createOptionMessage={(newValue) => `Select "${newValue}"`}
+      filterFunction={(filterValue, options) => {


@YuliaKrimerman I think I figured it out - Try this. Remove this filterFunction, and above where you removed the other code, add this:

// If we've selected an option that doesn't exist via isCreatable, include it in the options so it remains selected if (!selectOptions.some((option) => option.value === value)) { selectOptions.push({ value, content: value }); }

That ensures that our "newly created" item is in the list once it is selected, but disappears from the list if it is deselected.

YuliaKrimerman · 2024-09-20T13:10:07Z

So I ended up removing the stuff you suggested to remove and not adding the code you proposed because it looks like it was creating this pre selected empty field. At the current state everything looks like it's working as desired, let me know what you think

mturley · 2024-09-20T16:04:59Z

@YuliaKrimerman without the code I proposed, when I type "istio" and click the "Select 'istio'" option, the dropdown appears as if nothing is selected even though the value has updated (the confirm button in the row becomes enabled).

I do notice the issue you pointed out with my suggestion, it's because I forgot to make sure value is defined before pushing it as an option. This should fix it:

  // If we've selected an option that doesn't exist via isCreatable, include it in the options so it remains selected
  if (value && !selectOptions.some((option) => option.value === value)) {
    selectOptions.push({ value, content: value });
  }

YuliaKrimerman · 2024-09-20T17:12:23Z

@mturley Yep, you're right, now it works good. Pushed the latests and ready for re-review

mturley

Perfect, thanks @YuliaKrimerman !!

openshift-ci · 2024-09-20T17:23:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

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:

~~frontend/OWNERS~~ [mturley]

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

YuliaKrimerman added 2 commits September 18, 2024 10:34

saving work

cb89299

working fix

3eed1aa

openshift-ci bot requested review from alexcreasy and lucferbux September 18, 2024 14:47

YuliaKrimerman commented Sep 18, 2024

View reviewed changes

mturley requested changes Sep 18, 2024

View reviewed changes

openshift-ci bot assigned mturley Sep 18, 2024

addressed all comments except entry duplicates

8e4b63f

YuliaKrimerman commented Sep 19, 2024

View reviewed changes

YuliaKrimerman added 3 commits September 19, 2024 14:36

Updated the Tooltip

257acc6

fixed test to match new behaviour

6058971

format

645d361

mturley reviewed Sep 19, 2024

View reviewed changes

comments

016f481

comment2

186b5df

mturley approved these changes Sep 20, 2024

View reviewed changes

openshift-ci bot added the lgtm label Sep 20, 2024

openshift-ci bot added the approved label Sep 20, 2024

openshift-merge-bot bot merged commit 83472f4 into opendatahub-io:main Sep 20, 2024
6 checks passed

YuliaKrimerman deleted the RHOAIENG-12025 branch October 10, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MR Permissions - Group and Project dropdown issues #3219

MR Permissions - Group and Project dropdown issues #3219

YuliaKrimerman commented Sep 18, 2024

YuliaKrimerman Sep 18, 2024

mturley Sep 18, 2024

codecov bot commented Sep 18, 2024 •

edited

Loading

mturley left a comment

mturley Sep 18, 2024

mturley Sep 18, 2024

mturley Sep 18, 2024

mturley Sep 18, 2024

mturley Sep 18, 2024

YuliaKrimerman Sep 19, 2024

YuliaKrimerman Sep 19, 2024

mturley Sep 19, 2024

mturley Sep 18, 2024

jeff-phillips-18 Sep 19, 2024 •

edited

Loading

mturley Sep 19, 2024 •

edited

Loading

YuliaKrimerman Sep 19, 2024

YuliaKrimerman Sep 19, 2024

mturley Sep 19, 2024

YuliaKrimerman Sep 19, 2024

mturley Sep 19, 2024

mturley Sep 19, 2024

mturley left a comment

mturley Sep 19, 2024

YuliaKrimerman commented Sep 20, 2024

mturley commented Sep 20, 2024

YuliaKrimerman commented Sep 20, 2024

mturley left a comment

openshift-ci bot commented Sep 20, 2024

MR Permissions - Group and Project dropdown issues #3219

MR Permissions - Group and Project dropdown issues #3219

Conversation

YuliaKrimerman commented Sep 18, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

mturley Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuliaKrimerman commented Sep 20, 2024

mturley commented Sep 20, 2024

YuliaKrimerman commented Sep 20, 2024

mturley left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 20, 2024

codecov bot commented Sep 18, 2024 •

edited

Loading

jeff-phillips-18 Sep 19, 2024 •

edited

Loading

mturley Sep 19, 2024 •

edited

Loading