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 input borders when CustomSelectDropdown is opened #7419

Merged

Conversation

mendrew
Copy link
Contributor

@mendrew mendrew commented Aug 21, 2024


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

Описание

Мы не отправляли внутреннее значение placement из CustomSelect в CustomSelectDropdown потому и в некоторых ситуациях onPlacementChange callback не вызывался, потому что при длительном взаимодейтсвии с CustomSelect, внутренее значение могло быть top, при этом в CustomSelectDropdown передавался постоянно prop placement="button", в итоге usePlacementChangeCallback не вызывал onPlacementChange callback.

Изменение

Передаем внутренне состояние placement в CustomSelectDropdown, вместо popupDirection.

Тут есть эффект того, что изменение пропа placement после первого рендера не поменяет направление дропдауна.
Но это фича, которая не особо имеет смысл.
Контролировать placement через проп в CustomSelect не очень получится, он принимает значения 'top' | 'bottom' и по умолчанию уже bottom, то есть изменить его можно будет только на top, причём, только если дропдаун направлен вниз.
В то же время уже и сейчас, без этого фикса, если направление dropdown автоматически высчиталось как 'top', то сделать его bottom через prop popupDirection не получится.

Думаю, это нормально, что через свойство placement мы задаём значение для первого рендера, ведь дальше направлением дропдауна, если места под него нету снизу, управляет floatin-ui.

@github-actions github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Aug 21, 2024
Copy link
Contributor

github-actions bot commented Aug 21, 2024

size-limit report 📦

Path Size
JS 378.18 KB (-0.05% 🔽)
JS (gzip) 114.72 KB (-0.06% 🔽)
JS (brotli) 94.2 KB (-0.08% 🔽)
JS import Div (tree shaking) 1.39 KB (0%)
CSS 306.22 KB (0%)
CSS (gzip) 39.28 KB (0%)
CSS (brotli) 31.49 KB (0%)

Copy link

codesandbox-ci bot commented Aug 21, 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 Aug 21, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Aug 21, 2024

👀 Docs deployed

Commit 0ac845a

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.47%. Comparing base (ead90ac) to head (0ac845a).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7419      +/-   ##
==========================================
- Coverage   94.47%   94.47%   -0.01%     
==========================================
  Files         375      375              
  Lines       11123    11120       -3     
  Branches     3650     3651       +1     
==========================================
- Hits        10509    10506       -3     
  Misses        614      614              
Flag Coverage Δ
unittests 94.47% <100.00%> (-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.

@mendrew mendrew marked this pull request as ready for review August 21, 2024 12:38
@mendrew mendrew requested a review from a team as a code owner August 21, 2024 12:38
@mendrew mendrew modified the milestone: v6.6.0 Aug 21, 2024
@mendrew mendrew requested a review from SevereCloud August 22, 2024 12:26
Мы не отправляли внутреннее значение placement из CustomSelect
в CustomSelectDropdown потому и в некоторых ситуациях
onPlacementChange callback не вызывался

Шаги для воспроизведения ситуации
- Пролистать экран вверх, чтобы опции от CustomSelect не помещались снизу, и встали сверху
- Свернуть опции от CustomSelect кликом по компоненту
- Пролистать экран вниз, чтобы опции от CustomSelect начали помещаться снизу и открыть их нажатием на компонент
Since we can pass it directly to the Popper
@mendrew mendrew force-pushed the mendrew/7413/fix/CustomSelect/borders-on-dropdown-open branch from 2724914 to 58bf8d5 Compare August 22, 2024 12:34
@mendrew mendrew marked this pull request as draft August 22, 2024 12:43
@mendrew mendrew marked this pull request as ready for review August 22, 2024 12:58
@mendrew
Copy link
Contributor Author

mendrew commented Aug 22, 2024

@SevereCloud отребейзил и поправил import type { согласно новому правилу в eslint. ⚡ 0ac845a

@mendrew mendrew merged commit 5c1a8d8 into master Aug 23, 2024
40 of 52 checks passed
@mendrew mendrew deleted the mendrew/7413/fix/CustomSelect/borders-on-dropdown-open branch August 23, 2024 09:36
@vkcom-publisher
Copy link
Contributor

❌ Patch

Не удалось автоматически применить исправление на ветке 6.5-stable.

Дальнейшие действия выполняют контрибьютеры из группы @VKCOM/vkui-core

Чтобы изменение попало в ветку 6.5-stable, выполните следующие действия:

  1. Создайте новую ветку от 6.5-stable и примените изменения используя cherry-pick
git stash # опционально
git fetch origin 6.5-stable
git checkout -b patch/pr7419 origin/6.5-stable

git cherry-pick --no-commit 5c1a8d83ccefd6246b4afa6797cdf7c70a998860
git checkout HEAD **/__image_snapshots__/*.png
git diff --quiet HEAD || git commit --no-verify --no-edit
  1. Исправьте конфликты, следуя инструкциям из терминала
  2. Отправьте ветку на GitHub и создайте новый PR с веткой 6.5-stable (установка лейбла не требуется!)
git push --set-upstream origin patch/pr7419
gh pr create --base 6.5-stable --title "patch: pr7419" --body "- patch #7419"

mendrew added a commit that referenced this pull request Aug 23, 2024
…pdown is opened (#7419)

Передаем внутренне состояние placement в CustomSelectDropdown, вместо popupDirection.

Мы не отправляли внутреннее значение placement из CustomSelect в CustomSelectDropdown потому и в некоторых ситуациях onPlacementChange callback не вызывался, потому что при длительном взаимодейcтвии с CustomSelect, внутренее значение могло быть top, при этом в CustomSelectDropdown передавался постоянно prop placement="botton", в итоге usePlacementChangeCallback не вызывал onPlacementChange callback.

Тут есть эффект того, что изменение пропа placement после первого рендера не поменяет направление дропдауна.
Но это фича, которая не особо имеет смысл.
Контролировать placement через проп в CustomSelect не очень получится, он принимает значения 'top' | 'bottom' и по умолчанию уже bottom, то есть изменить его можно будет только на top, причём, только если дропдаун направлен вниз.
В то же время уже и сейчас, без этого фикса, если направление dropdown автоматически высчиталось как 'top', то сделать его bottom через prop popupDirection не получится.

Думаю, это нормально, что через свойство placement мы задаём значение для первого рендера, ведь дальше направлением дропдауна, если места под него нету снизу, управляет floatin-ui.
# Conflicts:
#	packages/vkui/src/components/ChipsSelect/ChipsSelect.test.tsx
#	packages/vkui/src/components/CustomSelect/CustomSelect.test.tsx
@mendrew mendrew mentioned this pull request Aug 23, 2024
mendrew added a commit that referenced this pull request Aug 23, 2024
…pdown is opened (#7419) (#7433)

Передаем внутренне состояние placement в CustomSelectDropdown, вместо popupDirection.

Мы не отправляли внутреннее значение placement из CustomSelect в CustomSelectDropdown потому и в некоторых ситуациях onPlacementChange callback не вызывался, потому что при длительном взаимодейcтвии с CustomSelect, внутренее значение могло быть top, при этом в CustomSelectDropdown передавался постоянно prop placement="botton", в итоге usePlacementChangeCallback не вызывал onPlacementChange callback.

Тут есть эффект того, что изменение пропа placement после первого рендера не поменяет направление дропдауна.
Но это фича, которая не особо имеет смысл.
Контролировать placement через проп в CustomSelect не очень получится, он принимает значения 'top' | 'bottom' и по умолчанию уже bottom, то есть изменить его можно будет только на top, причём, только если дропдаун направлен вниз.
В то же время уже и сейчас, без этого фикса, если направление dropdown автоматически высчиталось как 'top', то сделать его bottom через prop popupDirection не получится.

Думаю, это нормально, что через свойство placement мы задаём значение для первого рендера, ведь дальше направлением дропдауна, если места под него нету снизу, управляет floating-ui.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
None yet
4 participants