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

fix(CustomSelect): refactor logic of empty value #7822

Merged
merged 7 commits into from
Oct 31, 2024

Conversation

EldarMuhamethanov
Copy link
Contributor

@EldarMuhamethanov EldarMuhamethanov commented Oct 24, 2024


  • Unit-тесты
  • e2e-тесты
  • Документация фичи
  • Release notes

Описание

Сейчас в компоненте CustomSelect есть проблема с тем, что у option может быть value="", но у нас это значение используется для невыбранного состояния. Нужно придумать как разделить этий кейсы.

Изменения

  • Первым делом разделил кейсы невыбранного состояния и value="". Решил, что для невыбранного состояния можно использовать null. undefined не подойдет, так как он используется для Uncontrolled компонентов.
  • Следующая проблема была в нативном селекте: если в качестве value в него прокинуть null, то он конвертирует его в пустую строку(""), что мешает использовать null в качестве невыбранного состояния. Решил, что можно использовать специальную строку для отображения невыбранного состояния
    const NOT_SELECTED = '__vkui_internal_CustomSelect_not_selected__';
    
    Вообще все это нужно, чтобы срабатывало событие onChange при сбросе выбранного значения через кнопку clear. Выходит, что получаются следующие типы:
    export type SelectValue = Exclude<
      React.SelectHTMLAttributes<HTMLSelectElement>['value'],
      undefined
    > | null; // Значения value, которые можно прокинуть в компонент. Все возможные типы минус undefined плюс null
    
    export type NativeSelectValue = Exclude<SelectValue, null>; // Значения нативного селекта. Тип SelectValue минус null. 
    
    Также добавил функции, чтобы ремапить из SelectValue в NativeSelectValue и наоборот
  • Добавил прокидывание второго параметра в onChange. Туда будет прокидываться новое value. В документации указал, что рекомендуется использовать именно второе значение. Сделал вторым параметром, чтобы не ломать обратную совместимость, возможно в будущем стоит поменять местами с первым.
    onChange?: (e: ChangeEvent<HTMLSelectElement>, newValue: SelectValue) => void;
    
  • В документации к value и defaultValue написал про использование null вместо undefined для указания невыбранного состояния
  • Также добавил проверки при миксовании Controlled и Uncontrolled компонента. Добавил варнинг в таком случае.
  • Поправил тесты, которые сломались
  • Поправил примеры в документации

UPD

  • Поскольку данные изменения все равно скорее breaking change, поэтому решил, что можно в данной задаче в целом избавиться от прокидывания в onChange события ChangeEvent

Release notes

Исправления

Улучшения

  • Select: для указания невыбранного состояния теперь необходимо использовать null вместо undefined или пустой строки. То же самое относится и к CustomSelect и NativeSelect

BREAKING CHANGE

  • Select: в колбэк onChange вместо ChangeEvent теперь прокидывается новое значение. То же самое относится и к CustomSelect и NativeSelect

    Пример
    <Select
      id="select-type-select-id"
      value={selectType}
      placeholder="Не задан"
      options={selectTypes}
    - onChange={e => setSelectType(e.target.value)}
    + onChange={(newType) => setSelectType(newType)}
    />

@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner October 24, 2024 07:45
Copy link
Contributor

github-actions bot commented Oct 24, 2024

size-limit report 📦

Path Size
JS 382.42 KB (+0.27% 🔺)
JS (gzip) 122.38 KB (+0.33% 🔺)
JS (brotli) 101.35 KB (+0.31% 🔺)
JS import Div (tree shaking) 1.47 KB (0%)
CSS 342.2 KB (-0.01% 🔽)
CSS (gzip) 49.32 KB (-0.01% 🔽)
CSS (brotli) 39.95 KB (+0.01% 🔺)

Copy link

codesandbox-ci bot commented Oct 24, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

github-actions bot commented Oct 24, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Oct 24, 2024

👀 Docs deployed

Commit 7168752

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.17%. Comparing base (792e571) to head (7168752).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
.../vkui/src/components/CustomSelect/CustomSelect.tsx 97.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7822      +/-   ##
==========================================
- Coverage   95.17%   95.17%   -0.01%     
==========================================
  Files         376      376              
  Lines       11021    11041      +20     
  Branches     3662     3664       +2     
==========================================
+ Hits        10489    10508      +19     
- Misses        532      533       +1     
Flag Coverage Δ
unittests 95.17% <98.24%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Conflicts:
#	packages/vkui/src/components/CustomSelect/Readme.md
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk left a comment

Choose a reason for hiding this comment

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

Классная идея и реализация 👍 🔥


Я бы ещё явно в доке Select прописал про необходимость использовать null в качестве значения для не выбранной опции, чтобы не было сюрпризов.
Отдельной секцией, не только в пропе.

И примеры с null в доках Select, CustomSelect, NativeSelect.

packages/vkui/src/components/CustomSelect/CustomSelect.tsx Outdated Show resolved Hide resolved
packages/vkui/src/components/CustomSelect/Readme.md Outdated Show resolved Hide resolved
packages/vkui/src/components/NativeSelect/NativeSelect.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk left a comment

Choose a reason for hiding this comment

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

Класс!

Теперь можно будет аккуратно убрать dispatch событий onChange, чтобы не мимикрировать под нативный onChange

if (shouldTriggerOnChangeWhenControlledAndInnerValueIsOutOfSync) {
const event = new Event('change', { bubbles: true });
selectElRef.current?.dispatchEvent(event);

selectElRef.current?.dispatchEvent(event);

Но это уже отдельно можно сделать.

packages/vkui/src/components/CustomSelect/CustomSelect.tsx Outdated Show resolved Hide resolved
Co-authored-by: Andrey Medvedev <[email protected]>
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk left a comment

Choose a reason for hiding this comment

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

❤️ 💅

Copy link
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

🚀 💅

@EldarMuhamethanov EldarMuhamethanov merged commit ebb46a5 into master Oct 31, 2024
28 checks passed
@EldarMuhamethanov EldarMuhamethanov deleted the e.muhamethanov/7696/fix-empty-value-logic branch October 31, 2024 15:20
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.

[Bug]: Недетерминированное поведение CustomSelect и пустого value
3 participants