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): fix props.options and local state option syncronization #8365

Conversation

andrey-medvedev-vk
Copy link
Contributor

@andrey-medvedev-vk andrey-medvedev-vk commented Mar 13, 2025


  • Unit-тесты
  • e2e-тесты
  • Дизайн-ревью
  • Документация фичи
  • Release notes

Описание

В #8334 видно, что при асинхронной загрузке опций есть лаг между тем когда fetching уже не true, но опций ещё нет и мы в дропдауне, вместо лоадера, выводим Ничего не найдено.
Опций нет по той причине, что рендерим мы опции из локального стейта, который мы синхронизируем с props.options, а синхронизируем мы через useEffect.
Получается, что должен сработать useEffect прежде чем новые, загруженные опции пользователя, переданные через props.options, попадут в рендер дропдауна.

Изменения

  • Убран локальный стейт options, вместо этого options это результат фильтрации props.options, обёрнутой в useMemo.
  • Изменилась логика проверки необходимости вызова onChange при клике на опцию из списка.
    Раньше она была основана на проверке по индексам опций в списке, типа selectedOptionIndex, индекс брался изoptions, отфильтрованной версии props.options.
    Теперь такая логика не работает для принятия решения по вызову props.onChange, потому что options фильтруются быстрее, а значит ссылаться на опцию по индексу опасно, индекс опции уже может поменяться между кликом по опции и моментом, когда мы начнём принимать решение о вызове onChange.
    Переехали на проверку по значению nativeSelectValue/prevNativeSelectValue, а не по индексу.

Release notes

Исправления

  • CustomSelect: исправлена задержки вывода опций, переданных в props.options при асинхронной загрузке

Use local options which we use as filtered data with useMemo

No more usage of options as state, since it takes at least one render
to update local option in useEffect and show options in the dropdown.
It's not working properly when we fetch options with opened dropdown.
Since it shows for a moment that there is no options, while they are
here, just local state is not updated.
@github-actions github-actions bot added the ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Mar 13, 2025
Copy link

codesandbox-ci bot commented Mar 13, 2025

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 Mar 13, 2025

size-limit report 📦

Path Size
JS 398.6 KB (+0.05% 🔺)
JS (gzip) 120.93 KB (+0.05% 🔺)
JS (brotli) 99.45 KB (+0.06% 🔺)
JS import Div (tree shaking) 1.56 KB (0%)
CSS 348.96 KB (0%)
CSS (gzip) 43.23 KB (0%)
CSS (brotli) 34.45 KB (0%)

Copy link
Contributor

github-actions bot commented Mar 13, 2025

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Mar 13, 2025

👀 Docs deployed

Commit 806e079

Copy link

codecov bot commented Mar 13, 2025

Codecov Report

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

Project coverage is 95.42%. Comparing base (a5681eb) to head (1202e04).

Files with missing lines Patch % Lines
.../vkui/src/components/CustomSelect/CustomSelect.tsx 97.77% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8365   +/-   ##
=======================================
  Coverage   95.42%   95.42%           
=======================================
  Files         409      409           
  Lines       11649    11651    +2     
  Branches     3859     3862    +3     
=======================================
+ Hits        11116    11118    +2     
  Misses        533      533           
Flag Coverage Δ
unittests 95.42% <97.77%> (+<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.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@andrey-medvedev-vk andrey-medvedev-vk marked this pull request as ready for review March 14, 2025 09:46
@andrey-medvedev-vk andrey-medvedev-vk requested a review from a team as a code owner March 14, 2025 09:46
@BlackySoul
Copy link
Contributor

2025-03-14.17.43.21.mov

Такую штуку заметила, что при повторном поиске ранее выбранной опции она на некоторое время подсвечивается и потом сбрасывается 🤔

@andrey-medvedev-vk
Copy link
Contributor Author

2025-03-14.17.43.21.mov

Такую штуку заметила, что при повторном поиске ранее выбранной опции она на некоторое время подсвечивается и потом сбрасывается 🤔

О, интересно, спасибо, посмотрю! Тоже, видимо с обновлением индексов дело. Пока переведу в драфт.

@andrey-medvedev-vk andrey-medvedev-vk marked this pull request as draft March 14, 2025 10:59
@andrey-medvedev-vk
Copy link
Contributor Author

2025-03-14.17.43.21.mov

Такую штуку заметила, что при повторном поиске ранее выбранной опции она на некоторое время подсвечивается и потом сбрасывается 🤔

@BlackySoul
Оказывается это воспроизводится и у нас в мастере. Причем в Хроме воспроизводится, а в Фаерфоксе это не поймать.
Шаги для воспроизведения.

  1. Выбрать опцию из списка,
  2. Переместить мышку на инпут
  3. Открыть дропдуан с клавиатуры (в примере выше путём ввода текста)
  4. Появится список и выбранная опция будет в фокусе, а потом фокус пропадёт.

Связано с тем, что когда у нас мышка на инпуте, то после открытия дропдауна тригерится событие onMouseLeave причём target у него это та самая опция, бывшая в фокусе, а relatedTarget (target на который курсор перешёл) это инпут. А на onMouseLeave у нас завязана логика подсветки элемента в фокусе (программный ховер).

Поправил, с помощью проверки, что мышка не двигалась в момент события onMouseLeave.
40d3f88956a147626b1b3de39087090f6f64a84d^1...mendrew/CustomSelect/8334/fix-options-synchronization

@andrey-medvedev-vk andrey-medvedev-vk marked this pull request as ready for review March 18, 2025 10:01
@andrey-medvedev-vk andrey-medvedev-vk removed the ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Mar 18, 2025
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.

🥇

каждый кто чинит CustomSelect автоматом становится героем) очень сложный компонент в 1000 строк

@andrey-medvedev-vk andrey-medvedev-vk requested review from BlackySoul and a team March 25, 2025 15:07
@andrey-medvedev-vk andrey-medvedev-vk merged commit 76c1680 into master Mar 26, 2025
53 checks passed
@andrey-medvedev-vk andrey-medvedev-vk deleted the mendrew/CustomSelect/8334/fix-options-synchronization branch March 26, 2025 07:39
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.

[Bug]: Рассинхрон в CustomSelect
4 participants