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

refactor(select): reworked selectMobile component with apply feature #599

Merged
merged 27 commits into from
Apr 13, 2023

Conversation

NikitaRodyukov
Copy link
Contributor

Опишите проблему

Невозможно использовать хук useSelectWithApply вместе с компонентом SelectMobile, также нельзя контролировать состояние выбора снаружи компонента

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2023

🦋 Changeset detected

Latest commit: 4964d33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@alfalab/core-components-select Major
@alfalab/core-components-input-autocomplete Patch
@alfalab/core-components-intl-phone-input Patch
@alfalab/core-components-picker-button Patch
@alfalab/core-components-select-with-tags Patch
@alfalab/core-components-table Patch
@alfalab/core-components-tabs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coveralls
Copy link

coveralls commented Mar 22, 2023

Pull Request Test Coverage Report for Build 4655550796

  • 107 of 138 (77.54%) changed or added relevant lines in 10 files are covered.
  • 37 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.3%) to 80.602%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/select/src/components/base-select-mobile/Component.tsx 11 12 91.67%
packages/select/src/presets/useSelectWithApply/options-list-with-apply/Component.tsx 4 6 66.67%
packages/select/src/components/base-select-mobile/footer/Component.tsx 6 10 60.0%
packages/select/src/presets/useSelectWithApply/options-list-with-apply/footer/Component.tsx 4 9 44.44%
packages/select/src/components/select-modal-mobile/Component.tsx 5 14 35.71%
packages/select/src/components/virtual-options-list/Component.tsx 58 68 85.29%
Files with Coverage Reduction New Missed Lines %
packages/select/src/components/select-modal-mobile/Component.tsx 1 15.38%
packages/textarea/src/Component.tsx 4 86.44%
packages/bottom-sheet/src/component.tsx 9 83.87%
packages/select/src/presets/useSelectWithApply/options-list-with-apply/Component.tsx 23 8.93%
Totals Coverage Status
Change from base Build 4509342968: 0.3%
Covered Lines: 7823
Relevant Lines: 8807

💛 - Coveralls

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@NikitaRodyukov NikitaRodyukov self-assigned this Mar 22, 2023
@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@reme3d2y
Copy link
Contributor

Тут есть нюанс, что по дефолту мобильный селект должен работать с "подтверждением".
Мы можем зашить хук внутрь моб. селекта, но оставить возможность переопределить его снаружи, если нужно?

@reme3d2y
Copy link
Contributor

Тут выбор работает "на заднем плане", а кнопки не работают

const OPTIONS = Array.from({ length: 100 }).map((_, i) => ({
    key: i.toString(),
    content: `Neptunium ${i}`,
}));

render(() => {
    const [selected, setSelected] = React.useState([]);

    const handleChange = ({ selectedMultiple }) => {
        setSelected(selectedMultiple.map((option) => option.key));
    };

    return (
        <div style={{ width: document.body.clientWidth < 450 ? '100%' : 320 }}>
            <SelectModalMobile
                multiple
                allowUnselect={true}
                options={OPTIONS}
                placeholder='С подтверждением'
                Option={BaseOption}
                block={true}
                selected={selected}
                onChange={handleChange}
            />
        </div>
    );
});

@reme3d2y
Copy link
Contributor

Нам бы вообще избавиться от "прикопанных" компонентов

image

@reme3d2y
Copy link
Contributor

Еще есть SelectModalMobile, нужно чекнуть, что в нем все будет работать корректно

@reme3d2y
Copy link
Contributor

Сейчас список открывается на 5.5 опций, как в десктопе. Это поведение нужно сделать отключаемым для мобилки.
Тебе нужно будет сравнить поведение с "components/base-select-mobile/options-list", т.к. если ты уберешь обрезание, то скорее всего возникнут проблемы со скроллом.
Когда будешь тестить - чекни варианты с большим кол-вом опций (100+) и с 1-2 опциями. И нужно убедиться, что VirtualOptionsList работает корректно

image

const OPTIONS = [
    { key: '1', content: 'Neptunium' },
    { key: '2', content: 'Plutonium' },
    { key: '3', content: 'Americium' },
    { key: '4', content: 'Curium' },
    { key: '5', content: 'Berkelium' },
    { key: '6', content: 'Californium' },
    { key: '7', content: 'Einsteinium' },
    { key: '8', content: 'Fermium' },
];

render(() => {
    const [selected, setSelected] = React.useState([]);

    const handleChange = ({ selectedMultiple }) => {
        setSelected(selectedMultiple.map((option) => option.key));
    };

    return (
        <div style={{ width: document.body.clientWidth < 450 ? '100%' : 320 }}>
            <SelectMobile
                allowUnselect={true}
                options={OPTIONS}
                placeholder='С подтверждением'
                Option={BaseOption}
                block={true}
                {...useSelectWithApply({
                    options: OPTIONS,
                    selected,
                    onChange: handleChange,
                })}
            />
        </div>
    );
});

…-mobile

Убрал компоненты в base-select-mobile, отправил хук useSelectWithApply внутрь компонента, теперь на
мобилках правильно работает подтверждение применения с множественным выбором
@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@reme3d2y
Copy link
Contributor

Никита, ты герой! 🔥

С BottomSheet вроде все ок работает. С модалкой есть пару проблем:

  1. Список сразу скороллится в конец
  2. Есть синяя обводка у футера
  3. Странная анамация открытия (анимируется только шапка)

packages/select/src/presets/useSelectWithApply/hook.tsx Outdated Show resolved Hide resolved
@@ -46,6 +46,8 @@ export const VirtualOptionsList = ({
const [visibleOptionsInvalidateKey, setVisibleOptionsInvalidateKey] = useState(0);
const prevHighlightedIndex = usePrevious(highlightedIndex) || -1;

const numberOfVisibleOptions = visibleOptionsFromProps === 0 ? 12 : visibleOptionsFromProps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Screencast.from.2023-03-31.17-22-42.webm

С виртуальным списком что-то не то, два скролла сейчас

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Насчет VirtualOptionsList нет идей что можно сделать чтобы пофиксить дабл скролл, такое поведение только с BottomSheet, с модалкой вроде ок

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@v-gevak
Copy link
Contributor

v-gevak commented Apr 10, 2023

Screenshot from 2023-04-10 13-29-54
В этой демке перестали проставляться галочки рядом с выбранной опцией

@core-ds-bot
Copy link
Collaborator

Собрана новая демка.

@NikitaRodyukov NikitaRodyukov merged commit ef0159d into master Apr 13, 2023
8 checks passed
@NikitaRodyukov NikitaRodyukov deleted the refactor/select-mobile branch April 13, 2023 09:23
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.

None yet

8 participants