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.

Sign up for GitHub

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

Jump to bottom

MR Permissions - Group and Project dropdown issues #3219

Merged
merged 8 commits into from
Sep 20, 2024
Merged

MR Permissions - Group and Project dropdown issues #3219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb89299
saving work
YuliaKrimerman Sep 16, 2024
3eed1aa
working fix
YuliaKrimerman Sep 18, 2024
8e4b63f
addressed all comments except entry duplicates
YuliaKrimerman Sep 19, 2024
257acc6
Updated the Tooltip
YuliaKrimerman Sep 19, 2024
6058971
fixed test to match new behaviour
YuliaKrimerman Sep 19, 2024
645d361
format
YuliaKrimerman Sep 19, 2024
016f481
comments
YuliaKrimerman Sep 20, 2024
186b5df
comment2
YuliaKrimerman Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ...tests__/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistryPermissions.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,12 @@ describe('MR Permissions', () => {

usersTab.findAddGroupButton().click();

groupTable.findNameSelect().fill('new-example-mr-group');
cy.findByText('Create "new-example-mr-group"').click();
// Type the group name into the input
groupTable.findNameSelect().type('new-example-mr-group');

// Wait for the dropdown to appear and select the option
cy.contains('new-example-mr-group', { timeout: 10000 }).should('be.visible').click();

groupTable.findSaveNewButton().click();

cy.wait('@addGroup').then((interception) => {
Expand Down
10 changes: 4 additions & 6 deletions frontend/src/components/TypeaheadSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ export interface TypeaheadSelectProps extends Omit<SelectProps, 'toggle' | 'onSe

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

const defaultFilterFunction = (filterValue: string, options: TypeaheadSelectOption[]) =>
options.filter((o) => String(o.content).toLowerCase().includes(filterValue.toLowerCase()));

Expand Down Expand Up @@ -212,12 +211,10 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
const onInputClick = () => {
if (!isOpen) {
openMenu();
setTimeout(() => {
textInputRef.current?.focus();
}, 100);
} else if (isFiltering) {
closeMenu();
}
setTimeout(() => {
textInputRef.current?.focus();
}, 100);
};
Comment on lines 213 to 218
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@mturley mturley Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.


const selectOption = (
Expand Down Expand Up @@ -401,6 +398,7 @@ const TypeaheadSelect: React.FunctionComponent<TypeaheadSelectProps> = ({
onSelect={handleSelect}
onOpenChange={(open) => !open && closeMenu()}
toggle={toggle}
shouldFocusFirstItemOnOpen={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as suggested from Jeff

ref={innerRef}
{...props}
>
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/concepts/roleBinding/RoleBindingPermissionsNameInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,10 @@ const RoleBindingPermissionsNameInput: React.FC<RoleBindingPermissionsNameInputP
const displayName = isProjectSubject ? namespaceToProjectDisplayName(option, projects) : option;
return { value: displayName, content: displayName };
});

if (value && !typeAhead.includes(value)) {
// 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 });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return (
<TypeaheadSelect
selectOptions={selectOptions}
Expand All @@ -68,6 +67,7 @@ const RoleBindingPermissionsNameInput: React.FC<RoleBindingPermissionsNameInputP
}
}}
placeholder={placeholderText}
createOptionMessage={(newValue) => `Select "${newValue}"`}
/>
);
};
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/concepts/roleBinding/RoleBindingPermissionsTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ const RoleBindingPermissionsTableRow: React.FC<RoleBindingPermissionsTableRowPro
<Tooltip
content={
<div>
This group is created by default. You can add users to this group via the API.
This group is created by default. You can add users to this group in OpenShift
user management, or ask the cluster admin to do so.
</div>
}
>
Expand Down
Loading