-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(Flex): improve className withGaps calculation #8360
fix(Flex): improve className withGaps calculation #8360
Conversation
size-limit report 📦
|
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. |
e2e tests |
👀 Docs deployed
Commit 9777934 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8360 +/- ##
==========================================
+ Coverage 95.42% 95.44% +0.01%
==========================================
Files 409 410 +1
Lines 11649 11671 +22
Branches 3859 3862 +3
==========================================
+ Hits 11116 11139 +23
+ Misses 533 532 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Решение крутое! Если думать в масштабе, то смущает MutationOberserv
– этот полифилл нужен для старых браузеров, а старый браузер у пользователя может быть по двум причинам:
- у него старое устройство, которое не поддерживается новыми версиями браузера
- осознанно не обновляет
Если говорить про п.1, то тут нужно учитывать, что старое устройство будет слабей по ресурсам. Flex
может использоваться достаточно часто – помимо React, который будет хранить в памяти каждый из них в своём VDOM, память ещё будет разрастаться экземплярами MutationObserver
.
Будет ещё проблема при SSR – при гидрации будет ошибка, т.к. класс withGaps
будет навешиваться после 1-го рендера. Ну и reflow будет.
Вообще withGaps
, если так подумать, не нужен ведь. Даже если будет один элемент, то ему можно продолжать добавлять отступы и компенсировать их на родителе. Как минимум, можно учесть кейс, что будет 0 потомков через :empty
. Давай попробуем вот так решить проблему:
@supports not (inset: 0) {
.host:not(:empty) {
margin-block-start: calc(
-1 * var(--vkui_internal--row_gap) + var(--vkui_internal--flex_original_margin_block)
);
margin-inline-start: calc(
-1 * var(--vkui_internal--column_gap) + var(--vkui_internal--flex_original_margin_inline)
);
}
/* stylelint-disable-next-line @project-tools/stylelint-atomic, selector-max-universal */
.host.host > * {
margin-block-start: var(--vkui_internal--row_gap);
margin-inline-start: var(--vkui_internal--column_gap);
}
}
Этот функционал был добавлен намерено, так как исправлял баг (тут добавлен #7492), так что проверять на
Понимаю, что решение далеко от идеала, но других вариантов нет(во всяком случае, я не нашел) |
...можно было догадаться, что этот случай нужно иначе порешать установив В примере #7492 видим, что у вложенного нет <Flex gap="xs">
{new Array(3).fill(3).map((i) => (
<div
style={{ width: '36px', height: '36px', borderRadius: '50%', border: '1px solid red' }}
key={i}
>
<Flex
align="center"
justify="center"
style={{
width: '36px',
height: '36px',
border: '1px solid blue',
'--vkui_internal--column_gap': '0px',
'--vkui_internal--row_gap': '0px'
}}
>
VKUI
</Flex>
</div>
))}
</Flex> Когда у тебя во PS: пример фолбека можно глянуть тут https://gavinmcfarland.github.io/flex-gap-polyfill/, в целом мы так и делаем, за исключением бага, что |
Действительно в том кейсе проблема в наследовании. Собственно, добавил в css в |
думаю нужно будет упомянуть фикс в release notes, что по новой решили #7492 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅
Описание
Сейчас в компоненте
Flex
есть проверкаconst withGaps = Children.count(children) > 1 && gap;
, которая определяет нужно ли включать полифил gap для старых браузеров(добавляет отрицательные маржины для контейнера). Эта проверка не всегда работает корректно, так как неточно определяет сколько дочерних элементов будет к контейнера. Например, прокидывание элементов, обернутых однимFragment
. Исходя из проверкиwithGaps
будетfalse
, так как по сутиchildren
один. Нужно придумать как сделать корректное определение количества непосредственных детей компонентаИзменения
useWithGaps
, который определяет значениеwithGaps
на основанииchildren
у контейнера. Определяет он это по факту после отрисовки компонента. Также он отслеживает изменение количества непосредственных потомковflex
-контейнера черезMutationObserver
flex + gap
, добавил проверку с помощьюCSS.supports('(inset: 0)')
- аналогичная проверка есть в cssuseMutationObserver
, так чтобы он принимал объектoptions
для настройкиMutationObserver
Fragment
, так как это больше не будет актуальнымUPD
В итоге решили откатить эти изменения. Был поправлен баг с наследованием gap во вложенных Flex. Классней withGaps был выпилен. Подробнее можно прочитать в комменте
Release notes
Улучшения