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.

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

feat(TreeSelect): added TreeSelect unstable component and new list hooks #1090

Merged
merged 14 commits into from
Jan 22, 2024

Conversation

IsaevAlexandr
Copy link
Contributor

Completely rewritten to version with hooks.

Prev pr: #1039

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@IsaevAlexandr IsaevAlexandr changed the base branch from main to next November 14, 2023 10:48
@Mishnya
Copy link
Contributor

Mishnya commented Nov 20, 2023

I see you're using the react-window.

This library has a problem. In rtl, the list for an item when dragging sets the style right: 0 instead right: undefined, which causes the item to move to the right until the end of the screen.

To fix this you can manually set right: undefined to the item. I didn't find any more problems with with this bug and fix but maybe we can find an alternative to this library (which has not been updated since December 2018).

@IsaevAlexandr IsaevAlexandr changed the title feat: new list proposal feat(TreeSelect): added TreeSelect unstable component and new list hooks Nov 27, 2023
@IsaevAlexandr
Copy link
Contributor Author

I see you're using the react-window.

This library has a problem. In rtl, the list for an item when dragging sets the style right: 0 instead right: undefined, which causes the item to move to the right until the end of the screen.

To fix this you can manually set right: undefined to the item. I didn't find any more problems with with this bug and fix but maybe we can find an alternative to this library (which has not been updated since December 2018).

The default virtualization solution is no longer assumed. react-window is just for example here, because it is already in project dependencies

@IsaevAlexandr IsaevAlexandr force-pushed the feature/new-list-hooks branch from df03760 to 3dcb75f Compare January 18, 2024 11:04
@IsaevAlexandr IsaevAlexandr force-pushed the feature/new-list-hooks branch from 3dcb75f to 4b6d553 Compare January 18, 2024 11:44
border-radius: var(--g-list-item-border-radius-l, 8px);
}
&_radius_xl {
border-radius: var(--g-list-item-border-radius-xl, 8px);
Copy link
Contributor

Choose a reason for hiding this comment

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

public var should be without any size, just --g-list-item-border-radius

@@ -1,6 +1,6 @@
export type ListItemId = string;

export type ListSizeTypes = 's' | 'm' | 'l' | 'xl';
export type ListItemSizeType = 's' | 'm' | 'l' | 'xl';
Copy link
Contributor

Choose a reason for hiding this comment

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

ListItemSize, "Type" suffix is redundant

src/unstable.ts Outdated
useListState as unstable_useListState,
useListFilter as unstable_useListFilter,
useListKeydown as unstable_useListKeydown,
} from './components/useList';
export * from './components/TreeSelect';
Copy link
Contributor

Choose a reason for hiding this comment

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

We keep TreeSelect stable?

height,
expanded,
style,
role = 'listitem',
Copy link
Contributor

Choose a reason for hiding this comment

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

role option more suitable here, it supports aria-selected attribute, while listitem doesn't

@IsaevAlexandr
Copy link
Contributor Author

/fixed


interface UseListStateProps extends Partial<ListState> {}

function useControlledState<T>(value: T, defaultValue: T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to be more precise useControlledState<T>(value: T | undefined, defaultValue: T)

Copy link
Contributor

@GermanVor GermanVor Jan 19, 2024

Choose a reason for hiding this comment

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

I would even write like this

function useControlledState<T>(value: T | undefined, defaultValue: T) {
    const [state, setState] = React.useState(value ?? defaultValue);
    const isControlled = value === undefined;

    return {value: value ?? state, setState, isControlled};
}

Comment on lines +19 to +21
const [innerValue, setInnerValue] = React.useState(defaultValue || []);

const value = valueProps || innerValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like useControlledState from here.

Suggested change
const [innerValue, setInnerValue] = React.useState(defaultValue || []);
const value = valueProps || innerValue;
const [value, setInnerValue] = useControlledState(valueProps, defaultValue || []);

useControlledState is good hook for library not only for useList area

Comment on lines 6 to 9
const [open, setOpenState] = React.useState(props.defaultOpen || false);
const {onOpenChange} = props;
const {onOpenChange, onClose} = props;
const isControlled = typeof props.open === 'boolean';
const openValue = isControlled ? (props.open as boolean) : open;
Copy link
Contributor

@GermanVor GermanVor Jan 19, 2024

Choose a reason for hiding this comment

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

Suggested change
const [open, setOpenState] = React.useState(props.defaultOpen || false);
const {onOpenChange} = props;
const {onOpenChange, onClose} = props;
const isControlled = typeof props.open === 'boolean';
const openValue = isControlled ? (props.open as boolean) : open;
const {
value: open,
isControlled,
setState: setOpenState,
} = useControlledState(props.open, false);
const {onOpenChange, onClose} = props;

Comment on lines +68 to +71
if (externalItems !== prevItems) {
setItems(filterItemsFn(filter, externalItems));
setPrevItems(externalItems);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

prevItems looks like extra:

React.useEffect(() => {
    setItems(filterItemsFn(filter, externalItems));
}, [externalItems])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's new recommended way to not use useEffect things in code - https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Why they do not use React.useRef for prev state


return () => items;
},
[filterItem, filterItems],
Copy link
Contributor

@GermanVor GermanVor Jan 19, 2024

Choose a reason for hiding this comment

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

may be wrap filterItem and filterItems in the React.useRef like this:

const filterItemsRef = React.useRef(filterItems);
filterItemsRef.current = filterItems;

const filterItemRef = React.useRef(filterItem);
filterItemRef.current = filterItem;

const filterItemsFn = React.useCallback(() => {
    ...
    filterItemRef.current() or filterItemsRef.current()
    ....
}, []);

to remove them from dependency list - otherwise, if the user does not memorize them, then there is no point in debouncedFn

@amje amje merged commit 36893b3 into gravity-ui:next Jan 22, 2024
3 of 4 checks passed
amje pushed a commit that referenced this pull request Feb 1, 2024
amje pushed a commit that referenced this pull request Feb 6, 2024
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.

6 participants