Skip to content

Conversation

@mvolkmann
Copy link
Collaborator

No description provided.

@mvolkmann mvolkmann marked this pull request as draft May 3, 2024 13:40

.member-selector-dialog .name-search-field {
width: 350px;
margin-right: 3rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think there was too much horizontal space between the "Name" input and the "Filter Type" dropdown, so I removed this margin-right property.

display: flex;
flex-direction: row;
gap: 0.5rem;
gap: 1rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need a bit more space between each of the form controls.

@@ -1,3 +1,7 @@
.member-selector-dialog .custom-tenure-picker {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This sets the width of a DatePicker that was added for selecting a custom tenure value.

@@ -1,14 +1,20 @@
import { differenceInMonths } from 'date-fns';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is used to determine if a member has been at OCI for at least the selected tenure value.

import PropTypes from 'prop-types';
import {
AppBar,
Autocomplete,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I combined some imports.

import SearchIcon from '@mui/icons-material/Search';
import Autocomplete from '@mui/material/Autocomplete';
import FormControl from '@mui/material/FormControl';
import { DatePicker } from '@mui/x-date-pickers';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is used to specify a custom tenure start date.

const [directReportsOnly, setDirectReportsOnly] = useState(false);

const [selectableMembers, setSelectableMembers] = useState([]);
const [tenure, setTenure] = useState(initialFilter?.tenure || Tenures.All);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The initial value of the tenure state is the one specific in initialFilter. But if that isn't present, default to Tensures.All.

);

// Exclude members that don't have the selected tenure.
if (tenure === Tenures.Custom) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a custom tenure date was selected, keep only the members whose start date was on or before that date.

const start = new Date(member.startDate);
return start <= customTenure;
});
} else if (tenure !== Tenures.All) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a specific tenure value like "5 Years" was selected, keep only the members that have been at OCI at least that long.

)
}}
/>
<FormControl className="filter-type-select">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This Select is not new. I just relabelled it to make its purpose more clear.

<TextField
{...params}
variant="outlined"
label="Filter Members"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This input is for a value of the type specified in the Select above. I think labeling it "Filter Value" instead of "Filter Members" makes this more clear.

onChange={(_, value) => setFilter(value)}
/>
</div>
<Checkbox
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This checkbox is not new. It was just moved in the code.

disabled={selectableMembers.length === 0}
/>
</FormGroup>
<FormGroup className="dialog-form-group">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This checkbox is not new. It was just moved in the code.

label="Direct reports only"
/>
)}
<FormControl className="filter-type-select">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the new Select for specifying a desired tenure.

/>
}
label="Direct reports only"
{tenure === Tenures.Custom && (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a date picker for selecting a custom start date for tenure.

/>
)}
</div>
<Checkbox
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This checkbox was not deleted. It was just moved up in the code.

expect(memberList).toHaveLength(initialState.memberProfiles.length);

const filterField = await screen.findByRole('combobox', {
name: /filter members/i
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed some labels in the code, so these UI tests needed to be updated to find elements based on their new labels.

@mvolkmann mvolkmann requested a review from jackkeller May 3, 2024 15:05
@mvolkmann mvolkmann requested review from mkimberlin and vhscom May 3, 2024 15:05
@mvolkmann mvolkmann marked this pull request as ready for review May 6, 2024 14:16
Copy link
Collaborator

@jackkeller jackkeller left a comment

Choose a reason for hiding this comment

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

🎉

@mvolkmann mvolkmann merged commit a2f85fc into develop May 6, 2024
@mkimberlin mkimberlin deleted the feature-2285-member-selector-tenure branch August 27, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants