Skip to content

Conversation

@mvolkmann
Copy link
Collaborator

No description provided.

className,
style
}) => {
const isControlled = !!selected && Array.isArray(selected);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously this component maintained its own state of selected members in addition to allowing them to be passed in by a component that also maintains that same state. I changed this to require the selected members to be passed in using the selected prop. The function specified by the onChange prop is called every time the selection of members changes so the parent component can update its state. This simplifies the code and works much better with our custom hook useQueryParameters.

Copy link
Contributor

@vhscom vhscom May 2, 2024

Choose a reason for hiding this comment

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

This component is a little tricky I'm noticing, as both the earlier and the current implementations do not seem to update the member selection when the selected prop changes (e.g. during runtime as data becomes available from the server) as I'd expected. Typically with controlled components, changes to the controlling props (like selected in this case) should cause a re-render of the child. Just noting the observation for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was a little tricky. Now it works like a dream. ✨

import React, { useContext, useState } from 'react';
import React, { useContext, useRef, useState } from 'react';

import { Autocomplete, Button, TextField, Typography } from '@mui/material';
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 MUI imports.

listHeight={300}
onChange={selected => setSelectedMembers(selected)}
onChange={setSelectedMembers}
selected={selectedMembers}
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 MemberSelector component now requires the selected prop.

const [showRadar, setShowRadar] = useState(false);

const processedQPs = useRef(false);
useQueryParameters(
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 main point of this PR.

S78901
S78901 previously approved these changes May 2, 2024
@S78901
Copy link
Contributor

S78901 commented May 2, 2024

You may want to check for snapshots or what is causing the Gradle build to fail.

@mvolkmann mvolkmann requested a review from S78901 May 2, 2024 19:59
@mvolkmann mvolkmann merged commit fca1991 into develop May 2, 2024
@mkimberlin mkimberlin deleted the feature-2309-team-skills-report-query-parameters 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