Browse Source

ref(settings): Use `CompositeSelect` for members filter (#47678)

**Before ——**
<img width="673" alt="image"
src="https://user-images.githubusercontent.com/44172267/233216600-7540e689-135f-498f-a078-56134a0d743e.png">

**After ——**
<img width="414" alt="image"
src="https://user-images.githubusercontent.com/44172267/233216652-4319537a-d04a-4c5b-8dc2-ece3efc66850.png">
Vu Luong 1 year ago
parent
commit
aa1e832c54

+ 63 - 147
static/app/views/settings/organizationMembers/components/membersFilter.tsx

@@ -1,9 +1,7 @@
-import styled from '@emotion/styled';
-
-import Checkbox from 'sentry/components/checkbox';
-import Switch from 'sentry/components/switchButton';
+import {SelectOption} from 'sentry/components/compactSelect';
+import {CompositeSelect} from 'sentry/components/compactSelect/composite';
+import {IconSliders} from 'sentry/icons';
 import {t} from 'sentry/locale';
-import {space} from 'sentry/styles/space';
 import {OrgRole} from 'sentry/types';
 import {MutableSearch} from 'sentry/utils/tokenizeSearch';
 
@@ -14,12 +12,6 @@ type Props = {
   className?: string;
 };
 
-type BooleanFilterProps = {
-  label: string;
-  onChange: (value: boolean | null) => void;
-  value: boolean | null;
-};
-
 type Filters = {
   has2fa: boolean | null;
   isInvited: boolean | null;
@@ -27,160 +19,84 @@ type Filters = {
   ssoLinked: boolean | null;
 };
 
-const getBoolean = (list: string[]) =>
-  Array.isArray(list) && list.length
-    ? list && list.map(v => v.toLowerCase()).includes('true')
-    : null;
+const getBooleanValue = (list: string[]) => {
+  if (!Array.isArray(list) || !list.length) {
+    return 'all';
+  }
+
+  return list && list.map(v => v.toLowerCase()).includes('true') ? 'true' : 'false';
+};
+
+const booleanOptions = [
+  {value: 'all', label: t('All')},
+  {value: 'true', label: t('True')},
+  {value: 'false', label: t('False')},
+];
 
-function MembersFilter({className, roles, query, onChange}: Props) {
+function MembersFilter({roles, query, onChange}: Props) {
   const search = new MutableSearch(query);
 
   const filters = {
     roles: search.getFilterValues('role') || [],
-    isInvited: getBoolean(search.getFilterValues('isInvited')),
-    ssoLinked: getBoolean(search.getFilterValues('ssoLinked')),
-    has2fa: getBoolean(search.getFilterValues('has2fa')),
+    isInvited: getBooleanValue(search.getFilterValues('isInvited')),
+    ssoLinked: getBooleanValue(search.getFilterValues('ssoLinked')),
+    has2fa: getBooleanValue(search.getFilterValues('has2fa')),
   };
 
-  const handleRoleFilter = (id: string) => () => {
-    const roleList = new Set(
-      search.getFilterValues('role') ? [...search.getFilterValues('role')] : []
-    );
-
-    if (roleList.has(id)) {
-      roleList.delete(id);
-    } else {
-      roleList.add(id);
-    }
-
-    const newSearch = search.copy();
-    newSearch.setFilterValues('role', [...roleList]);
-    onChange(newSearch.formatString());
-  };
-
-  const handleBoolFilter = (key: keyof Filters) => (value: boolean | null) => {
+  const handleBoolFilter = (key: keyof Filters) => (opt: SelectOption<string>) => {
     const newQueryObject = search.copy();
     newQueryObject.removeFilter(key);
-    if (value !== null) {
-      newQueryObject.setFilterValues(key, [Boolean(value).toString()]);
+    if (opt.value !== 'all') {
+      newQueryObject.setFilterValues(key, [opt.value]);
     }
 
     onChange(newQueryObject.formatString());
   };
 
   return (
-    <FilterContainer className={className}>
-      <FilterHeader>{t('Filter By')}</FilterHeader>
-
-      <FilterLists>
-        <FilterList>
-          <h3>{t('User Role')}</h3>
-          {roles.map(({id, name}) => (
-            <label key={id}>
-              <Checkbox
-                data-test-id={`filter-role-${id}`}
-                checked={filters.roles.includes(id)}
-                onChange={handleRoleFilter(id)}
-              />
-              {name}
-            </label>
-          ))}
-        </FilterList>
-
-        <FilterList>
-          <h3>{t('Status')}</h3>
-          <BooleanFilter
-            data-test-id="filter-isInvited"
-            onChange={handleBoolFilter('isInvited')}
-            value={filters.isInvited}
-            label={t('Invited')}
-          />
-          <BooleanFilter
-            data-test-id="filter-has2fa"
-            onChange={handleBoolFilter('has2fa')}
-            value={filters.has2fa}
-            label={t('2FA')}
-          />
-          <BooleanFilter
-            data-test-id="filter-ssoLinked"
-            onChange={handleBoolFilter('ssoLinked')}
-            value={filters.ssoLinked}
-            label={t('SSO Linked')}
-          />
-        </FilterList>
-      </FilterLists>
-    </FilterContainer>
-  );
-}
-
-function BooleanFilter({onChange, value, label}: BooleanFilterProps) {
-  return (
-    <label>
-      <Checkbox
-        aria-label={t('Enable %s filter', label)}
-        checked={value !== null}
-        onChange={() => onChange(value === null ? true : null)}
+    <CompositeSelect
+      triggerProps={{icon: <IconSliders />, size: 'md'}}
+      triggerLabel={t('Filter')}
+      maxMenuHeight="22rem"
+      size="sm"
+    >
+      <CompositeSelect.Region
+        multiple
+        label={t('Role')}
+        value={filters.roles}
+        options={roles.map(({id, name}) => ({value: id, label: name}))}
+        onChange={opts => {
+          const newSearch = search.copy();
+          newSearch.setFilterValues(
+            'role',
+            opts.map(opt => opt.value)
+          );
+          onChange(newSearch.formatString());
+        }}
       />
-      {label}
-      <Switch
-        aria-label={t('Toggle %s', label)}
-        isDisabled={value === null}
-        isActive={value === true}
-        toggle={() => onChange(!value)}
+      <CompositeSelect.Region
+        label={t('Invited')}
+        options={booleanOptions}
+        value={filters.isInvited}
+        onChange={handleBoolFilter('isInvited')}
+        closeOnSelect={false}
       />
-    </label>
+      <CompositeSelect.Region
+        label={t('2FA')}
+        options={booleanOptions}
+        value={filters.has2fa}
+        onChange={handleBoolFilter('has2fa')}
+        closeOnSelect={false}
+      />
+      <CompositeSelect.Region
+        label={t('SSO Linked')}
+        options={booleanOptions}
+        value={filters.ssoLinked}
+        onChange={handleBoolFilter('ssoLinked')}
+        closeOnSelect={false}
+      />
+    </CompositeSelect>
   );
 }
 
-const FilterContainer = styled('div')`
-  border-radius: 4px;
-  background: ${p => p.theme.background};
-  box-shadow: ${p => p.theme.dropShadowMedium};
-  border: 1px solid ${p => p.theme.border};
-`;
-
-const FilterHeader = styled('h2')`
-  border-top-left-radius: 4px;
-  border-top-right-radius: 4px;
-  border-bottom: 1px solid ${p => p.theme.border};
-  background: ${p => p.theme.backgroundSecondary};
-  color: ${p => p.theme.subText};
-  text-transform: uppercase;
-  font-size: ${p => p.theme.fontSizeExtraSmall};
-  padding: ${space(1)};
-  margin: 0;
-`;
-
-const FilterLists = styled('div')`
-  display: grid;
-  grid-template-columns: 100px max-content;
-  gap: ${space(3)};
-  margin: ${space(1.5)};
-  margin-top: ${space(0.75)};
-`;
-
-const FilterList = styled('div')`
-  display: grid;
-  grid-template-rows: repeat(auto-fit, minmax(0, max-content));
-  gap: ${space(1)};
-  font-size: ${p => p.theme.fontSizeMedium};
-
-  h3 {
-    color: #000;
-    font-size: ${p => p.theme.fontSizeSmall};
-    text-transform: uppercase;
-    margin: ${space(1)} 0;
-  }
-
-  label {
-    display: grid;
-    grid-template-columns: max-content 1fr max-content;
-    gap: ${space(0.75)};
-    align-items: center;
-    font-weight: normal;
-    white-space: nowrap;
-    height: ${space(2)};
-    margin: 0;
-  }
-`;
 export default MembersFilter;

+ 16 - 6
static/app/views/settings/organizationMembers/organizationMembersList.spec.jsx

@@ -7,6 +7,7 @@ import {
   screen,
   userEvent,
   waitFor,
+  within,
 } from 'sentry-test/reactTestingLibrary';
 
 import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
@@ -345,7 +346,7 @@ describe('OrganizationMembersList', function () {
     });
 
     await userEvent.click(screen.getByRole('button', {name: 'Filter'}));
-    await userEvent.click(screen.getByRole('checkbox', {name: 'Member'}));
+    await userEvent.click(screen.getByRole('option', {name: 'Member'}));
 
     expect(searchMock).toHaveBeenLastCalledWith(
       '/organizations/org-slug/members/',
@@ -355,15 +356,18 @@ describe('OrganizationMembersList', function () {
       })
     );
 
-    await userEvent.click(screen.getByRole('checkbox', {name: 'Member'}));
+    await userEvent.click(screen.getByRole('option', {name: 'Member'}));
 
     for (const [filter, label] of [
       ['isInvited', 'Invited'],
       ['has2fa', '2FA'],
       ['ssoLinked', 'SSO Linked'],
     ]) {
+      const filterSection = screen.getByRole('listbox', {name: label});
       await userEvent.click(
-        screen.getByRole('checkbox', {name: `Enable ${label} filter`})
+        within(filterSection).getByRole('option', {
+          name: 'True',
+        })
       );
 
       expect(searchMock).toHaveBeenLastCalledWith(
@@ -374,7 +378,11 @@ describe('OrganizationMembersList', function () {
         })
       );
 
-      await userEvent.click(screen.getByRole('checkbox', {name: `Toggle ${label}`}));
+      await userEvent.click(
+        within(filterSection).getByRole('option', {
+          name: 'False',
+        })
+      );
 
       expect(searchMock).toHaveBeenLastCalledWith(
         '/organizations/org-slug/members/',
@@ -385,7 +393,9 @@ describe('OrganizationMembersList', function () {
       );
 
       await userEvent.click(
-        screen.getByRole('checkbox', {name: `Enable ${label} filter`})
+        within(filterSection).getByRole('option', {
+          name: 'All',
+        })
       );
     }
   });
@@ -397,7 +407,7 @@ describe('OrganizationMembersList', function () {
     });
 
     await userEvent.click(screen.getByRole('button', {name: 'Filter'}));
-    await userEvent.click(screen.getByRole('checkbox', {name: 'Owner'}));
+    await userEvent.click(screen.getByRole('option', {name: 'Owner'}));
     await userEvent.click(screen.getByRole('button', {name: 'Filter'}));
 
     const owners = screen.queryAllByText('Owner');

+ 5 - 47
static/app/views/settings/organizationMembers/organizationMembersList.tsx

@@ -6,14 +6,11 @@ import styled from '@emotion/styled';
 import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
 import {resendMemberInvite} from 'sentry/actionCreators/members';
 import {redirectToRemainingOrganization} from 'sentry/actionCreators/organizations';
-import {Button} from 'sentry/components/button';
-import DeprecatedDropdownMenu from 'sentry/components/deprecatedDropdownMenu';
 import EmptyMessage from 'sentry/components/emptyMessage';
 import HookOrDefault from 'sentry/components/hookOrDefault';
 import Pagination from 'sentry/components/pagination';
 import {Panel, PanelBody, PanelHeader} from 'sentry/components/panels';
 import {ORG_ROLES} from 'sentry/constants';
-import {IconSliders} from 'sentry/icons';
 import {t, tct} from 'sentry/locale';
 import ConfigStore from 'sentry/stores/configStore';
 import {space} from 'sentry/styles/space';
@@ -262,22 +259,11 @@ class OrganizationMembersList extends AsyncView<Props, State> {
     // eslint-disable-next-line react/prop-types
     const renderSearch: RenderSearch = ({defaultSearchBar, value, handleChange}) => (
       <SearchWrapperWithFilter>
-        <DeprecatedDropdownMenu closeOnEscape>
-          {({getActorProps, isOpen}) => (
-            <FilterWrapper>
-              <Button icon={<IconSliders size="xs" />} {...getActorProps({})}>
-                {t('Filter')}
-              </Button>
-              {isOpen && (
-                <StyledMembersFilter
-                  roles={currentMember?.roles ?? ORG_ROLES}
-                  query={value}
-                  onChange={(query: string) => handleChange(query)}
-                />
-              )}
-            </FilterWrapper>
-          )}
-        </DeprecatedDropdownMenu>
+        <MembersFilter
+          roles={currentMember?.roles ?? ORG_ROLES}
+          query={value}
+          onChange={(query: string) => handleChange(query)}
+        />
         {defaultSearchBar}
       </SearchWrapperWithFilter>
     );
@@ -358,34 +344,6 @@ const SearchWrapperWithFilter = styled(SearchWrapper)`
   margin-top: 0;
 `;
 
-const FilterWrapper = styled('div')`
-  position: relative;
-`;
-
-const StyledMembersFilter = styled(MembersFilter)`
-  position: absolute;
-  right: 0;
-  top: 42px;
-  z-index: ${p => p.theme.zIndex.dropdown};
-
-  &:before,
-  &:after {
-    position: absolute;
-    top: -16px;
-    right: 32px;
-    content: '';
-    height: 16px;
-    width: 16px;
-    border: 8px solid transparent;
-    border-bottom-color: ${p => p.theme.backgroundSecondary};
-  }
-
-  &:before {
-    margin-top: -1px;
-    border-bottom-color: ${p => p.theme.border};
-  }
-`;
-
 const StyledPanelItem = styled('div')`
   display: grid;
   grid-template-columns: minmax(150px, auto) minmax(100px, 140px) 420px;