Skip to content

feat: OPTIC-1733: Standardize Dropdown Components Using LSE selector #7257

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.

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

Open
wants to merge 262 commits into
base: develop
Choose a base branch
from

Conversation

yyassi-heartex
Copy link
Contributor

@yyassi-heartex yyassi-heartex commented Mar 19, 2025

New unified Select component
image
This pull request primarily focuses on replacing existing Select components with the Select component from the @humansignal/ui library across multiple files. Additionally, it includes some refactoring and enhancements to improve code readability and functionality. Below are the most important changes:

UI Component Updates:

  • Replaced the existing Select components with Select from @humansignal/ui in various files, including Select.jsx, Pagination.tsx, Config.jsx, CreateProject.jsx, GeneralSettings.jsx, Forms.jsx, StorageForm.jsx, SampleDatasetSelect.tsx, ModelVersionSelector.jsx, and Select.jsx in the datamanager directory. [1] [2] [3] [4] [5] [6] [7] [8] Fbb5e0ebL1, [9]

Functional Enhancements:

  • Updated event handlers for Select components to use the new onChange signature provided by @humansignal/ui, replacing the previous event-based handlers. [1] [2] [3] [4] [5] [6]

Code Refactoring:

  • Added useMemo hooks to optimize the computation of options for Select components, ensuring better performance and readability. [1] [2] [3]

CSS and Layout Adjustments:

  • Modified CSS styles to support the new Select component layout, including changes in uikit.css to use display: flex instead of display: block.

Miscellaneous:

  • Minor adjustments to imports and code structure to maintain consistency and improve code clarity. [1] [2] [3] [4]

These changes collectively enhance the consistency and maintainability of the codebase by standardizing the use of the Select component from @humansignal/ui and optimizing related code.

yyassi-heartex and others added 30 commits March 3, 2025 09:27
…s/label-studio into fb-optic-1748/ui-improvements
…748/ui-improvements"

This reverts commit 291f6e3, reversing
changes made to 1c53cd9.
@borisheartex
Copy link
Contributor

borisheartex commented Apr 17, 2025

/fm sync

Workflow run

@yyassi-heartex
Copy link
Contributor Author

yyassi-heartex commented Apr 17, 2025

/git merge

Workflow run
Successfully merged: 10 files changed, 21 insertions(+), 18 deletions(-)

Comment on lines +82 to +87
setValue((prev = []) => {
if (isSelected) {
return (selectedOptions = [...prev.filter((v) => v !== val)]);
}
return (selectedOptions = [...prev, val]);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could cause an issue with the callback handler getting called after passing selectedOptions up to props.onChange. use a valueRef instead of the callback

)}
</button>
</PopoverTrigger>
<PopoverContent className="z-99999 min-w-full" align="start" data-testid="select-popup">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
<PopoverContent className="z-99999 min-w-full" align="start" data-testid="select-popup">
<PopoverContent align="start" data-testid="select-popup">

onSelect={() => {
children.forEach((child: SelectOption<T>) => {
const childVal = child?.value ?? child;
isOptionSelected ? _onChange(childVal, true) : _onChange(childVal, 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.

Suggested change
isOptionSelected ? _onChange(childVal, true) : _onChange(childVal, false);
_onChange(childVal, isOptionSelected);

@borisheartex
Copy link
Contributor

borisheartex commented Apr 18, 2025

/fm sync

Workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants